Skip to content

feat: share InformerManager when configuration is equal #954

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
metacosm wants to merge 3 commits into from
Closed

feat: share InformerManager when configuration is equal #954

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ca62b53
feat: share InformerManager when configuration is equal
metacosm Feb 17, 2022
d6d5806
fix: do not start/stop if not needed
metacosm Feb 17, 2022
a2bcf88
fix: format
metacosm Feb 17, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 9 additions & 5 deletions ...javaoperatorsdk/operator/processing/event/source/informer/ManagedInformerEventSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ public abstract class ManagedInformerEventSource<R extends HasMetadata, P extend

@SuppressWarnings("rawtypes")
private static final ConcurrentMap<ResourceConfigurationAsKey, InformerManager> managedInformers =
Copy link
Collaborator

@csviri csviri Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TBH, don't know what to think about this static context.
What is this magic number 17?

Copy link
Collaborator

@csviri csviri Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will be started and stopped multiple times. Don't like the facts that it is even shared between the controllers. It will be started and stopped multiple times.

Copy link
Collaborator

@csviri csviri Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it would deserve a Integration test with multiple controllers and multiple informers each.

Copy link
Collaborator Author

@metacosm metacosm Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, you're the one who wanted to share informers between sources… :)
We now avoid starting or stopping multiple times anyway.

Copy link
Collaborator

@csviri csviri Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pls add integration test for this, like when 2 reconcilers added with 2 Informers, so there is only one informer in the background.

Copy link
Collaborator

@csviri csviri Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And checking that everything is propoerly triggered after resource updated.

new ConcurrentHashMap<>(17);
new ConcurrentHashMap<>();

protected ManagedInformerEventSource(
MixedOperation<R, KubernetesResourceList<R>, Resource<R>> client, C configuration) {
Expand Down Expand Up @@ -51,14 +51,18 @@ protected InformerManager<R, C> manager() {

@Override
public void start() {
manager().start();
super.start();
if(!isRunning()) {
manager().start();
super.start();
}
}

@Override
public void stop() {
super.stop();
Copy link
Collaborator

@csviri csviri Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about the thread safety but probably ok this way

manager().stop();
if (isRunning()) {
super.stop();
manager().stop();
}
}

@SuppressWarnings("rawtypes")
Expand Down