feat: share InformerManager when configuration is equal #954
Conversation
metacosm
added
dependent-resources-epic
kind/feature
| @@ -141,4 +141,8 @@ public void put(ResourceID key, T resource) { | |||
| getSource(key.getNamespace().orElse(ANY_NAMESPACE_MAP_KEY)) | |||
| .ifPresent(c -> c.put(key, resource)); | |||
| } | |||
|
|
|||
| int numberOfSources() { | |||
| return sources.size(); | |||
There was a problem hiding this comment.
Should we just return an immutable map, this is not very useful outside of the tests.
There was a problem hiding this comment.
This is indeed just for tests, which is why it's package-protected.
| @@ -13,15 +18,31 @@ | |||
| extends CachingEventSource<R, P> | |||
| implements ResourceEventHandler<R> { | |||
|
|
|||
| @SuppressWarnings("rawtypes") | |||
| private static final ConcurrentMap<ResourceConfigurationAsKey, InformerManager> managedInformers = | |||
There was a problem hiding this comment.
TBH, don't know what to think about this static context.
What is this magic number 17?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
So it would deserve a Integration test with multiple controllers and multiple informers each.
There was a problem hiding this comment.
Well, you're the one who wanted to share informers between sources… :)
We now avoid starting or stopping multiple times anyway.
There was a problem hiding this comment.
Pls add integration test for this, like when 2 reconcilers added with 2 Informers, so there is only one informer in the background.
There was a problem hiding this comment.
And checking that everything is propoerly triggered after resource updated.
| } | ||
|
|
||
| @Override | ||
| public void stop() { | ||
| super.stop(); |
There was a problem hiding this comment.
Not sure about the thread safety but probably ok this way
|
BTW this is not exactly the same as sharing the Informer event source, we are just sharing the informer, so the events are still propagated twice in case two informers are created?! |
That's a good point but I'm actually unsure which behavior we would want. |
| protected ManagedInformerEventSource( | ||
| MixedOperation<R, KubernetesResourceList<R>, Resource<R>> client, C configuration) { | ||
| super(configuration.getResourceClass()); | ||
| initCache(configuration); | ||
| manager().initSources(client, configuration, this); |
There was a problem hiding this comment.
Won't this multiple time init sources? for the manager?
There was a problem hiding this comment.
Ehh we need a quite good quality intergation tests here IMHO. This is quite risky to do this way.
Well with sharing the event source it's one event per event source ( and one informer as here). So we are sharing here the informer not the event source. Not sure that is a good thing, especially quite hard to reason about, well maybe we need to get used to it. Basically we are sharing it because of the cache. From the point the event source is created the dependent resource only accesses the cache. There should be just one event that triggers the reconciliation from this point. If the primaryResourceMapper would be different for the same informer, that would be a different story. |
|
SonarCloud Quality Gate failed. |
|
Was thinking more of this, actually the idea is very good, just maybe we should come up with a design how exactly do this. Also not sure again on what layer, probably needed for informers but also for event source layer. |
Yes, this implementation doesn't feel complete and is problematic as shown by the failing tests… 😞 |
Fixes #942