Enhance event handling #407

lburgazzoli · 2021-04-21T16:57:40Z

This PR includes:

add lifecycle hooks to EventSource (Add lifecycle hooks to EventSource #368)
review DefaultEventSourceManager instance variable modifiers
make DefaultEventSourceManager::registerEventSource final as it is invoked by the constructor
DefaultEventSourceManager use containsKey instead of null check to determine if an event source is already registerd
add lifecycle hooks to the operator so i.e. watchers can be properly closed

Fixes #368

…t is invoked by the constructor

…k to determine if an event source is already registerd

metacosm · 2021-04-27T18:41:48Z

...in/java/io/javaoperatorsdk/operator/processing/event/internal/CustomResourceEventSource.java

@@ -155,7 +180,8 @@ public void onClose(WatcherException e) {
    if (e.isHttpGone()) {
      log.warn("Received error for watch, will try to reconnect.", e);
      try {
-        registerWatch();
+        close();


Should we really close all the watches? I guess there's no other option since we can't really know which watcher caused the exception…

yes, the issue was that in case of an error on of a single watcher, calling registerWatch would re-register all the watcher, potentially you could end up in duplicated watchers.

That was another thing I wondered about (using a List for the watchers as opposed to a Set but I'm not sure we have consistent equals/hashCode implementations for the Watch implementations)…

yes we need a way to improve how watchers are handled, I may try to have a look once this is done

metacosm · 2021-04-27T18:45:34Z

...re/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java

  }

  @Override
-  public <T extends EventSource> void registerEventSource(String name, T eventSource) {
+  public final void registerEventSource(String name, EventSource eventSource) {


Why did you make this method final?

because it is invoked by the constructor so either the class is marked as final or this method need to be final to avoid issues in classes extending DefaultEventSourceManager and overriding registerEventSource

Ah yes. I remember hearing about this but I have never run into such issues in practice… Have you?

yes, got bitten hard by it 😂 that's why I notice them

csviri · 2021-04-28T07:29:00Z

...re/src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java

@@ -17,9 +17,8 @@
  private static final Logger log = LoggerFactory.getLogger(DefaultEventSourceManager.class);

  private final ReentrantLock lock = new ReentrantLock();
-  private Map<String, EventSource> eventSources = new ConcurrentHashMap<>();
-  private CustomResourceEventSource customResourceEventSource;


Do you see it correctly that this has been CustomResourceEventSource is moved to the Operator? Why is that?
The intention was that this class will own all the custom resources related to a controller.
But are there particular reasons to move it?
The downside is that if we want to have other events,between event sources and controllers vs event source, but also access access CustomResourceEventSource (where we thinking to move the caching for example) will not be accessible in this manager.

The reason was that the method:

public void registerCustomResourceEventSource( CustomResourceEventSource customResourceEventSource) { this.customResourceEventSource = customResourceEventSource; this.customResourceEventSource.addedToEventManager(); }

Was not really meaningful as yes the customResourceEventSource was assigned to an instance of the DefaultEventManager but was never accesses by the manager.

A better option would have been either to use registerEventSource as any other event source or pass the customResourceEventSource as a constructor parameter to make it clear that the DefaultEventManager requires it whereas as today, it looks like an unrelated method.

The event handler for the CustomResourceEventSource was also set outside the event manager directly in the operator which is quite confusing.

I see, ok, yes this part definetelly needs improvements and refactorings. (Just like the others :) )
As I mentioned I would like to keep the event sources in one place, I think the constructor would make sense.
CustomResourceEventSource currently has quite special position, it's required always as an event source, so constructor is good ide also do express this.

Note that I will probably make some other improvements after this, getting back to work on #392

@csviri yep I think I got the reasoning and I do agree but for the sake of the lifecycle it was quite confusing so I opted to make at little bit more clear (i.e to have set-up / tear-down on the same side, in this case on the Operator). If we move it on the contructor then we can move its set-up/tear-down as part of the event manager.

lburgazzoli · 2021-04-28T08:44:36Z

@csviri @metacosm thx for the review

metacosm · 2021-04-28T08:53:31Z

Thank you!

lburgazzoli added 4 commits April 22, 2021 07:08

feat: add lifecycle hooks to EventSource (operator-framework#368)

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

e9e84d1

chore: review DefaultEventSourceManager instance variable modifiers

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.

GPG key ID: 4AEE18F83AFDEB23
Expired

Verified
Learn about vigilant mode

8106333

chore: make DefaultEventSourceManager::registerEventSource final as i…

b9b0317

…t is invoked by the constructor

chore: DefaultEventSourceManager use containsKey instead of null chec…

83b36c5

…k to determine if an event source is already registerd

lburgazzoli force-pushed the github-368 branch 2 times, most recently from 4498d40 to f3d2ade Compare April 22, 2021 08:52

feat: add lifecycle hooks to the operator framework

d7871f0

lburgazzoli force-pushed the github-368 branch from f3d2ade to d7871f0 Compare April 22, 2021 12:00

lburgazzoli marked this pull request as ready for review April 22, 2021 13:04

metacosm self-requested a review April 27, 2021 16:21

metacosm self-assigned this Apr 27, 2021

snowdrop-bot mentioned this pull request Apr 27, 2021

Enhance event handling snowdrop-zen/java-operator-sdk#13

Closed

metacosm reviewed Apr 27, 2021

View reviewed changes

metacosm approved these changes Apr 27, 2021

View reviewed changes

metacosm requested review from csviri and adam-sandor April 27, 2021 19:24

csviri reviewed Apr 28, 2021

View reviewed changes

csviri approved these changes Apr 28, 2021

View reviewed changes

metacosm merged commit 566e062 into operator-framework:master Apr 28, 2021

lburgazzoli deleted the github-368 branch April 28, 2021 09:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Enhance event handling #407

Enhance event handling #407

lburgazzoli commented Apr 21, 2021 •

edited

Loading

Uh oh!

metacosm Apr 27, 2021

Uh oh!

lburgazzoli Apr 27, 2021

Uh oh!

metacosm Apr 27, 2021

Uh oh!

lburgazzoli Apr 27, 2021

Uh oh!

metacosm Apr 27, 2021

Uh oh!

lburgazzoli Apr 27, 2021

Uh oh!

metacosm Apr 27, 2021 •

edited

Loading

Uh oh!

lburgazzoli Apr 27, 2021

Uh oh!

csviri Apr 28, 2021 •

edited

Loading

Uh oh!

lburgazzoli Apr 28, 2021 •

edited

Loading

Uh oh!

csviri Apr 28, 2021

Uh oh!

metacosm Apr 28, 2021

Uh oh!

lburgazzoli Apr 28, 2021

Uh oh!

lburgazzoli commented Apr 28, 2021

Uh oh!

metacosm commented Apr 28, 2021

Uh oh!

Enhance event handling #407

Enhance event handling #407

Conversation

lburgazzoli commented Apr 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metacosm Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

csviri Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lburgazzoli Apr 28, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lburgazzoli commented Apr 28, 2021

Uh oh!

metacosm commented Apr 28, 2021

Uh oh!

lburgazzoli commented Apr 21, 2021 •

edited

Loading

metacosm Apr 27, 2021 •

edited

Loading

csviri Apr 28, 2021 •

edited

Loading

lburgazzoli Apr 28, 2021 •

edited

Loading