Informer based CustomResourceEventSource and caching #581

csviri · 2021-10-05T14:32:54Z

No description provided.

lburgazzoli · 2021-10-05T15:34:33Z

...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java

-        executeBufferedEvents(uid);
-      }
+      eventBuffer.addEvent(event.getRelatedCustomResourceID(), event);
+      monitor.processedEvent(event.getRelatedCustomResourceID(), event);


I know that it was like that even before but wonder if that's correct as in fact here the event is added to a buffer for late processing. In case of failure, the event may also be put back to the buffer so I guess, processedEvent should be invoked only when the event has been delivered to the controller. Eventually we can also add metrics about how long it takes for an event from the generation to the delivery.

@metacosm am I missing something ?

That is probably a good point. The event is not processed at this time. We should probably take a look on this in a separate PR.
Note that we plan to remove the event from the context:
#538

so not sure how this part will look like after this issue. We might still receive events, but the whole event handling should be simplified.

Bumps [jandex-maven-plugin](https://github.com/wildfly/jandex-maven-plugin) from 1.1.1 to 1.2.1. - [Release notes](https://github.com/wildfly/jandex-maven-plugin/releases) - [Commits](wildfly/jandex-maven-plugin@1.1.1...1.2.1) --- updated-dependencies: - dependency-name: org.jboss.jandex:jandex-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

Bumps [mockito-core](https://github.com/mockito/mockito) from 3.12.4 to 4.0.0. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](mockito/mockito@v3.12.4...v4.0.0) --- updated-dependencies: - dependency-name: org.mockito:mockito-core dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

csviri · 2021-10-11T12:49:02Z

@lburgazzoli @metacosm pls review this

metacosm · 2021-10-11T18:47:17Z

...mework/src/main/java/io/javaoperatorsdk/operator/config/runtime/AnnotationConfiguration.java

  private ConfigurationService service;

  public AnnotationConfiguration(ResourceController<R> controller) {
    this.controller = controller;
-    this.annotation = Optional.ofNullable(controller.getClass().getAnnotation(Controller.class));
+    this.annotation = controller.getClass().getAnnotation(Controller.class);


An exception should be thrown here if the annotation isn't present on the ResourceController.

Not sure if I get this. Did I change something on the behavior?

Yes, now if the annotation is not present then you'll get an NPE. It's fine to simplify all over as long as we fail early and tell the user that they need the annotation here, I think.

metacosm · 2021-10-11T19:08:39Z

...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java

@@ -176,11 +163,14 @@ private boolean executeBufferedEvents(String customResourceUid) {
          newEventForResourceId,
          controllerUnderExecution,
          latestCustomResource.isPresent());
+      if (latestCustomResource.isEmpty()) {


What should be done in this case? Shouldn't we try to get the resource from the cluster?

So if we assume that we receiving events reliably this is going to be ok eventually, would not complicate the code. I've put there a warning because in this case actually is a user error with some custom event source, so there is a sign of it.
Did not change the behavior now, this is same as was before just added logging.

Maybe would create a separate issues where we can discuss this situtation.

metacosm · 2021-10-11T19:14:31Z

...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java

-    boolean newEventsExists = eventBuffer.newEventsExists(executionScope.getCustomResourceUid());
-    eventBuffer.putBackEvents(executionScope.getCustomResourceUid(), executionScope.getEvents());
+    boolean newEventsExists = eventBuffer
+        .newEventsExists(CustomResourceID.fromResource(executionScope.getCustomResource()));


Why are we building the id from the CR here but then use getCustomResourceID later?

metacosm · 2021-10-11T19:15:23Z

...framework-core/src/main/java/io/javaoperatorsdk/operator/processing/DefaultEventHandler.java

-    eventBuffer.putBackEvents(executionScope.getCustomResourceUid(), executionScope.getEvents());
+    boolean newEventsExists = eventBuffer
+        .newEventsExists(CustomResourceID.fromResource(executionScope.getCustomResource()));
+    eventBuffer.putBackEvents(executionScope.getCustomResourceID(), executionScope.getEvents());


It'd make sense to use a local variable for executionScope.getCustomResourceID() in this method

metacosm · 2021-10-11T19:16:45Z

operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/EventBuffer.java


-  /** @deprecated use {@link #addEvent(String, Event)} */
-  @Deprecated
  public void addEvent(Event event) {


Why isn't this not deprecated anymore?

I did not see why it was actually before. But this is going away in the subsequent PR. (The whole even buffer concept)

metacosm · 2021-10-11T19:27:18Z

...ava/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventFilterTest.java

@@ -80,13 +79,13 @@ public void eventFilteredByCustomPredicateAndGenerationAware() {
    cr.getMetadata().setGeneration(1L);
    cr.getStatus().setConfigMapStatus("1");

-    eventSource.eventReceived(Watcher.Action.MODIFIED, cr);
+    eventSource.eventReceived(ResourceAction.UPDATED, cr, null);


Shouldn't there be an old version of the CR here?

metacosm · 2021-10-11T19:28:05Z

...ava/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSourceTest.java

@@ -44,35 +43,40 @@ public void skipsEventHandlingIfGenerationNotIncreased() {
    TestCustomResource customResource1 = TestUtils.testCustomResource();
    customResource1.getMetadata().setFinalizers(List.of(FINALIZER));

-    customResourceEventSource.eventReceived(Watcher.Action.MODIFIED, customResource1);
+    customResourceEventSource.eventReceived(ResourceAction.UPDATED, customResource1, null);


Why is this null here?

metacosm · 2021-10-11T19:36:16Z

...ework-core/src/test/java/io/javaoperatorsdk/operator/processing/DefaultEventHandlerTest.java

@@ -222,7 +207,7 @@ public void successfulExecutionResetsTheRetry() {

  @Test
  public void scheduleTimedEventIfInstructedByPostExecutionControl() {
-    var testDelay = 10000l;
+    var testDelay = 10000L;


Why a capital L here and not in the next method?

metacosm · 2021-10-11T19:37:55Z

...framework-core/src/test/java/io/javaoperatorsdk/operator/processing/EventDispatcherTest.java

@@ -113,7 +114,7 @@ void updatesBothResourceAndStatusIfFinalizerSet() {
    when(customResourceFacade.replaceWithLock(testCustomResource)).thenReturn(testCustomResource);

    eventDispatcher.handleExecution(
-        executionScopeWithCREvent(Watcher.Action.MODIFIED, testCustomResource));
+        executionScopeWithCREvent(ResourceAction.UPDATED, testCustomResource));


Why not use a static import here as well?

metacosm · 2021-10-12T08:42:53Z

LGTM apart from the last annotation issue. Could you please rebase and squash before merging as well, making sure all the commit messages follow the conventional format?

This is a major change and backbone of v2. We change also how the Resources are addressed both internally and from event source with CustomResourceID. Additional improvements also added.

feat: This is a major change and backbone of v2. We change also how the Resources are addressed both internally and from event source with CustomResourceID. Additional improvements also added. * chore: renaming vars named k8sClient to kubernetsClient * chore(deps): bump jandex-maven-plugin from 1.1.1 to 1.2.1 (#592) Bumps [jandex-maven-plugin](https://github.com/wildfly/jandex-maven-plugin) from 1.1.1 to 1.2.1. - [Release notes](https://github.com/wildfly/jandex-maven-plugin/releases) - [Commits](wildfly/jandex-maven-plugin@1.1.1...1.2.1) --- updated-dependencies: - dependency-name: org.jboss.jandex:jandex-maven-plugin dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * chore(deps-dev): bump mockito-core from 3.12.4 to 4.0.0 (#591) Bumps [mockito-core](https://github.com/mockito/mockito) from 3.12.4 to 4.0.0. - [Release notes](https://github.com/mockito/mockito/releases) - [Commits](mockito/mockito@v3.12.4...v4.0.0) --- updated-dependencies: - dependency-name: org.mockito:mockito-core dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * feature: Build PR on v2 * chore(ci): use Java 17 * chore(ci): use only Temurin distribution * chore: add generics to PostExecutionControl to reduce IDEs noise (#594) * chore: polish the junit5 extension (#593) * feat: Use informers as CustomResourceEventSource backbone and cache Co-authored-by: Ioannis Canellos <iocanel@gmail.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Chris Laprun <metacosm@gmail.com> Co-authored-by: Luca Burgazzoli <lburgazzoli@users.noreply.github.com>

lburgazzoli reviewed Oct 5, 2021

View reviewed changes

This was linked to issues Oct 6, 2021

Remove getEventSource() method from Event class ? #469

Closed

Informer based CustomResourceEventSource #567

Closed

csviri marked this pull request as ready for review October 7, 2021 11:49

csviri requested review from metacosm, lburgazzoli and adam-sandor October 7, 2021 11:49

iocanel and others added 6 commits October 7, 2021 18:20

chore: renaming vars named k8sClient to kubernetsClient

2e8f219

feature: Build PR on v2

b99ff10

chore(ci): use Java 17

326f82f

chore(ci): use only Temurin distribution

98e3def

csviri self-assigned this Oct 11, 2021

csviri mentioned this pull request Oct 11, 2021

Informer based CustomResourceEventSource and caching snowdrop-zen/java-operator-sdk#46

Closed

snowdrop-bot mentioned this pull request Oct 11, 2021

Informer based CustomResourceEventSource and caching snowdrop-zen/java-operator-sdk#48

Closed

lburgazzoli added 2 commits October 11, 2021 14:47

chore: add generics to PostExecutionControl to reduce IDEs noise (#594)

746f0c1

chore: polish the junit5 extension (#593)

bb6c00e

csviri mentioned this pull request Oct 11, 2021

Removing events from context #596

Merged

metacosm reviewed Oct 11, 2021

View reviewed changes

csviri mentioned this pull request Oct 12, 2021

Refined Interface of EventSource and EventSourceManager #597

Merged

metacosm approved these changes Oct 12, 2021

View reviewed changes

csviri linked an issue Oct 12, 2021 that may be closed by this pull request

Use Caffeine instead of homegrown cache for CustomResourceCache #356

Closed

feat: Use informers as CustomResourceEventSource backbone and cache

76f4c00

This is a major change and backbone of v2. We change also how the Resources are addressed both internally and from event source with CustomResourceID. Additional improvements also added.

csviri force-pushed the informer-creventsource branch from a371629 to 76f4c00 Compare October 12, 2021 11:58

csviri merged commit 0aa5b49 into v2 Oct 12, 2021

csviri deleted the informer-creventsource branch October 12, 2021 12:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Informer based CustomResourceEventSource and caching #581

Informer based CustomResourceEventSource and caching #581

csviri commented Oct 5, 2021

lburgazzoli Oct 5, 2021

csviri Oct 6, 2021

csviri commented Oct 11, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

csviri Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm Oct 11, 2021

csviri Oct 12, 2021

metacosm commented Oct 12, 2021

Informer based CustomResourceEventSource and caching #581

Informer based CustomResourceEventSource and caching #581

Conversation

csviri commented Oct 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

csviri commented Oct 11, 2021

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

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

metacosm commented Oct 12, 2021