refactor: simplify handling of reused event sources #1513

metacosm · 2022-10-03T19:12:49Z

DependentResources that provide an EventSource should implement
EventSourceAware. If they want to reuse an already created event source
instead of providing their own, they need to select the appropriate
event source identified by the name specified by the useEventSourceNamed
method.

# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceDiscriminator.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceIDMatcherDiscriminator.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/ReconcileResult.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/BulkDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractCachingDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractSimpleDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/GenericKubernetesResourceMatcher.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowCleanupExecutor.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/workflow/WorkflowReconcileExecutor.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceRetriever.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventSource.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractSimpleDependentResourceTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/bulkdependent/BulkExternalDependentIT.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/ConfigMapDeleterBulkDependentResource.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/bulkdependent/external/ExternalBulkDependentResource.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multipledependentresource/MultipleDependentResourceReconciler.java

# Conflicts: # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/config/AnnotationControllerConfiguration.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/ResourceDiscriminator.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/DependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractEventSourceHolderDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractSimpleDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependent.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResource.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/kubernetes/KubernetesDependentResourceConfig.java # operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceRetriever.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/config/ControllerConfigurationOverriderTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResourceTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/external/AbstractSimpleDependentResourceTest.java # operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/dependent/workflow/AbstractWorkflowExecutorTest.java # operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/multipledependentresource/MultipleDependentResourceReconciler.java

DependentResources that provide an EventSource should implement EventSourceAware. If they want to reuse an already created event source instead of providing their own, they need to select the appropriate event source identified by the name specified by the useEventSourceNamed method.

sonarqubecloud · 2022-10-04T07:48:23Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
22 Code Smells

25.9% Coverage
0.0% Duplication

csviri

Made comments, I have mixed feeling of some parts, especially that it is not explicit to not use event source, but this is fine for now since all our dependents work like this. Just not really future proof. (Actually will dig how this works in controller runtime)

csviri · 2022-10-04T07:32:24Z

...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/EventSourceAware.java


-public interface EventSourceAware<P extends HasMetadata> {
+  Class<R> resourceType();


It's a little smelly that we have resourceType in both places (also in dependent resource), but no big deal.

Yeah, it's not ideal but this allows to have a default implementation in the interface…

csviri · 2022-10-04T07:36:27Z

...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/EventSourceAware.java

+import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
+import io.javaoperatorsdk.operator.processing.event.source.ResourceEventSource;
+
+public interface EventSourceAware<R, P extends HasMetadata> {


A couple of remarks with this:

This is in one hand an elegant solution to couple the event source related methods into one interface
on the other hand one of the primary reasons was to put these methods to Dependent Resource to couple it more, thus to express that a dependent resource ideally (an usually, so if not it is more of an corner case) should provide an event source. With this we are getting back to the position that basically event source is an optional thing in general.

We don't have parity with the @Dependent annotation, so while setting there the eventSource parameter won't have effect on the dependent if it does not implement this interface. (Not that big deal just pointing out).

Yes, on both. This was an initial attempt to see where that could lead. We can (and probably should) refine things.

csviri · 2022-10-04T07:54:02Z

...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/EventSourceAware.java

+import io.javaoperatorsdk.operator.api.reconciler.EventSourceContext;
+import io.javaoperatorsdk.operator.processing.event.source.ResourceEventSource;
+
+public interface EventSourceAware<R, P extends HasMetadata> {


not sure if EventSourceAware is the proper name, but cannot think anything better.

csviri · 2022-10-04T07:55:19Z

...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/EventSourceAware.java


-  void selectEventSources(EventSourceRetriever<P> eventSourceRetriever);
+  void useEventSourceNamed(String eventSourceName);


maybe useEventSourceByName ?

I'd rather use useEventSourceWithName then.

sounds good :)

csviri · 2022-10-04T07:57:28Z

...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/EventSourceAware.java


-  void selectEventSources(EventSourceRetriever<P> eventSourceRetriever);
+  void useEventSourceNamed(String eventSourceName);


What little feels smelly that ok actually most of our dependent resources use event sources to fill the cache on create/update. But in this interface it is explicit to use a different event source but implicit to not provide one. While one is configurable (by setting the name on @Dependent) other can be achieved only by overriding the default Optional<ResourceEventSource<R, P>> eventSource(EventSourceContext<P> context) method. This is smelly I think.

csviri · 2022-10-04T08:00:41Z

@metacosm would be good to create a separate PR on next just to see if integration test passing,
( change the CI later to run integration tests on all targets? )

metacosm · 2022-10-04T08:22:35Z

Let's merge #1479 with the understanding that #1504 and this PR need to be addressed after.

csviri · 2022-10-04T08:33:30Z

Thinking about this, what if we just replace the doNotProvideEventSource, with useEventSourceByName in dependent resource? would not be little better?

regarding the comments I made, will create a PR for that, how it would look like.

metacosm · 2022-10-04T08:35:16Z

Superseded by #1516

metacosm · 2022-10-04T08:36:14Z

Thinking about this, what if we just replace the doNotProvideEventSource, with useEventSourceByName in dependent resource? would not be little better?

regarding the comments I made, will create a PR for that, how it would look like.

Yes, was thinking about removing EventSourceAware altogether and put everything on DependentResource…

csviri · 2022-10-04T08:38:16Z

...ore/src/main/java/io/javaoperatorsdk/operator/api/reconciler/dependent/EventSourceAware.java

+   */
+  default Optional<ResourceEventSource<R, P>> eventSource(EventSourceContext<P> context) {
+    return getUsedEventSourceName().map(
+        name -> context.getEventSourceRetriever().getResourceEventSourceFor(resourceType(), name));


Hmm also this is a little smelly that the EventSourceContext is now meant to be just register event sources, and support registration, not to read it.

csviri added 30 commits September 27, 2022 09:42

feat: improvements on caching and dependent resources

6e42e53

wip

7e3119c

fix

ffc150b

kubernetes dependent resource configuration

7b16504

IT fix

7b442bc

fixed ITs

f5fd957

index based discriminator

d9d4a95

IT fix

924f162

wip

7eb4eb7

fixes from rebase from next

1f6b8b9

fix after rebase

c359557

event source provider to context

e9c8c7e

todo fixes

2042fd8

remove void discriminator

8f04146

rebase on next

bdd2d96

fix: bulk creation of dependent resource directly in abstract resource

378f463

wip

210034a

wip

7a56e1c

wip to start IT

bc4709c

fixes, progress

2e96cd9

wp

6892077

matcher

9d8055b

test passes

e222735

bulk dependent resource to an interface

e55592d

wip

b0a7227

test improvement

88f0053

note

fe33446

wip

65705d4

rebase on next

58c4096

increates test timeout

6a4109b

csviri and others added 14 commits September 29, 2022 16:31

bulk dependent resources (#1448)

7c66c05

fixes after merge

66882bc

feat: decouple event source from cache + list discriminator (#1378)

4792f60

bulk dependent resources (#1448)

99f4b10

fixes after merge

ba64ed8

controller fix

5a912a5

merge fixes

4576677

automatic event source selection

55c3796

fix

964cf5a

proper setting event source

9bad894

fix: avoid NPE if no dependent resources are passed

b86b6f7

metacosm self-assigned this Oct 3, 2022

metacosm requested a review from csviri October 3, 2022 19:12

docs: javadoc fix

6fa1b5d

csviri approved these changes Oct 4, 2022

View reviewed changes

Base automatically changed from optional-eventsource-on-dr to next October 4, 2022 08:25

metacosm mentioned this pull request Oct 4, 2022

Optional eventsource on dependent resources #1479

Merged

csviri force-pushed the next branch from 04aa2b7 to 9c1be9d Compare October 4, 2022 08:30

metacosm closed this Oct 4, 2022

metacosm deleted the optional-es-simplify branch October 4, 2022 08:36

csviri reviewed Oct 4, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: simplify handling of reused event sources #1513

refactor: simplify handling of reused event sources #1513

metacosm commented Oct 3, 2022

sonarqubecloud bot commented Oct 4, 2022

csviri left a comment

csviri Oct 4, 2022

metacosm Oct 4, 2022

csviri Oct 4, 2022

metacosm Oct 4, 2022

csviri Oct 4, 2022

csviri Oct 4, 2022

metacosm Oct 4, 2022

csviri Oct 4, 2022

csviri Oct 4, 2022

csviri commented Oct 4, 2022

metacosm commented Oct 4, 2022

csviri commented Oct 4, 2022 •

edited

Loading

metacosm commented Oct 4, 2022

metacosm commented Oct 4, 2022

csviri Oct 4, 2022


		public interface EventSourceAware<P extends HasMetadata> {
		Class<R> resourceType();


		void selectEventSources(EventSourceRetriever<P> eventSourceRetriever);
		void useEventSourceNamed(String eventSourceName);

refactor: simplify handling of reused event sources #1513

refactor: simplify handling of reused event sources #1513

Conversation

metacosm commented Oct 3, 2022

sonarqubecloud bot commented Oct 4, 2022

csviri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri commented Oct 4, 2022

metacosm commented Oct 4, 2022

csviri commented Oct 4, 2022 • edited Loading

metacosm commented Oct 4, 2022

metacosm commented Oct 4, 2022

Choose a reason for hiding this comment

csviri commented Oct 4, 2022 •

edited

Loading