refactor: rename internal package to source, moving LifecycleAware #716
Conversation
EventSourceManager now owns the EventProcessor which also now owns the retry TimerSourceEvent since it's the only user of that functionality. We could expose methods on EventHandler to schedule retries if needed.
| @@ -88,11 +89,14 @@ private EventProcessor(ResourceCache<R> resourceCache, ExecutorService executor, | |||
| this.retry = retry; | |||
| this.resourceCache = resourceCache; | |||
| this.metrics = metrics != null ? metrics : Metrics.NOOP; | |||
| this.eventMarker = eventMarker; | |||
| this.eventMarker = new EventMarker(); | |||
| this.retryAndRescheduleTimerEventSource = new TimerEventSource<>(); | |||
There was a problem hiding this comment.
I don't know, but the thinking was that EventSourceManager encapsulates all the event sources, so manages also the lifecycle etc of the. This breaks that logic.
There was a problem hiding this comment.
I understand but that makes the code more complex. I tend to think that things that are only needed in one spot should be managed there.
There was a problem hiding this comment.
This is kinda a double-edged sword, since on the other hand it brings complexity in terms of managing the life cycle of an event source in the processor, which is kinda smell if we have already a manager for them. Before it was managed in a cohesive way in one place.
There was a problem hiding this comment.
The problem is that EventManager and EventProcessor should really be a single object as is evidenced by their tied lifecycle and the need to set a reciprocal relations between them. If we insist on splitting them, then they should each manage the parts that only they use.
|
|
||
| public class Controller<R extends HasMetadata> implements Reconciler<R>, | ||
| LifecycleAware, EventSourceInitializer<R> { | ||
| private final Reconciler<R> reconciler; | ||
| private final ControllerConfiguration<R> configuration; | ||
| private final KubernetesClient kubernetesClient; | ||
| private EventSourceManager<R> eventSourceManager; | ||
| private EventProcessor<R> eventProcessor; |
There was a problem hiding this comment.
The idea as desribed here is that the controller maneges the lifecycle of every other component top level component:
#655
So it is the highest level agregate, therefore will be no multiple layers for managing LifecycleAware
There was a problem hiding this comment.
The problem is that it doesn't really do anything with it, it's just needed by the event manager so I don't think we should expose it since the lifecycle of the EventProcessor is tied to the EventManager (you cannot have one without the other). Making EventManager handle the EventProcessor life cycle also allows to avoid the rather ugly setters and prevent forgetting to start/stop one or the other.
If the Controller was using the EventProcessor directly, it would make sense to keep there. At the very least, if we wanted to keep the top-level aggregate concept, the EventManager should retrieve the EventProcessor from the Controller (or vice-versa) and that's actually what I tried doing yesterday without complicating things even more…
There was a problem hiding this comment.
But in general the Controller does not do anything just instantiates and starts/stops the major components. The circular dependency is ugly, but the EventManager is just a class to encapsulate a list of EventSources. For me that it creates a central component, probably the most complex part is quite a smell.
There was a problem hiding this comment.
Except that EventSourceManager and EventProcessor are not really separate components. Again, one cannot exist without the other. If we want to keep them separate, we should make it as cleanly as possible and I think the current solution is cleaner because it avoids the mess of having incomplete objects for no good reason. Maybe we should combine EventSourceManager and EventProcessor, renaming them to EventManager?
There was a problem hiding this comment.
The problem with that is even now the EventProcessor is too big, with bunch of logic, what little bugs me. Actually wanted to somehow break it down to more components.
There was a problem hiding this comment.
It is indeed quite complex, but some things are 😄
It doesn't make sense to split things that are not coherent if we end up with more complexity to manage the different pieces. I could see the TimerEventSource moving back to EventSourceManager but I really think that we should tie the lifecycle of EventSourceManager and EventProcessor by making EventSourceManager handle the processor's lifecycle because, again, the manager cannot work without the processor.
There was a problem hiding this comment.
Then the event source manager will handle the lifecycle of event processor?
ok that is fine by me, we can evaluate further later.
| controllerResourceEventSource = new ControllerResourceEventSource<>(controller); | ||
| this.eventProcessor = new EventProcessor<>(this); |
There was a problem hiding this comment.
The event source manager should not instantiate the processor. I that the controller manages the lifecycle is much cleaner. This is what was moved out as part of the design changes.
There was a problem hiding this comment.
This doesn't make things cleaner: look at how ugly the setEventHandler / setEventManager mess is. You cannot use the EventProcessor without the EventManager and vice-versa. They should be created complete and not half way having to use a setter to inject the missing part after they're built, imo.
There was a problem hiding this comment.
except for those :D
package re-naming and moving lifecycle aware