Skip to content
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.

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

Summary of Proposed Changes for Naming and Related Refactoring for V2 #655

Closed
csviri opened this issue Nov 4, 2021 · 4 comments · Fixed by #646
Closed

Summary of Proposed Changes for Naming and Related Refactoring for V2 #655

csviri opened this issue Nov 4, 2021 · 4 comments · Fixed by #646
Assignees
Milestone

Comments

@csviri
Copy link
Collaborator

csviri commented Nov 4, 2021

Based on comments and other remarks, but also from the usage experience, for v2 we plan to do changes in the public and Internal APIs, mainly some structural and naming changes on the main classes. Some of these changes are also result of the functional changes of v2 (like removing events from context). This issues is a summary of current planned renamings and the related open questions. (see also the linked PR)

We would be more then happy to receive some feedback, if the new model and naming is intuitive enough.

Public API Changes:

  1. ResourceController -> Reconciler - as mentioned on related issues the Controller is a higher level concept and it's defintelly higher level in controller-runtime for example (https://book.kubebuilder.io/architecture.html ), what in our case ResourceController was doing in v1 is implementing reconciliation logic (and defining event sources). So we renamed it to Reconciler.

  2. Removed init(EventSourceManager) method from Reconciler. The original intention was to allow to do some initializations and mainly register event sources by implementing this method. However, it is only used to register event sources and access the CustomResourceEventSource - this is required in some corner cases in our experience. So we removed the init method and created a new interace EventSourceInitializer with prepareEventSources(EventSourceRegistry<T> eventSourceRegistry) to implement. So if Reconciler want's to register event sources needs to implement this method. Hopefully this better describes the domain. (We might further evaluate event source registration approach, since Reconciler might not be the best place to do that.)

  3. @Controller -> @ControllerConfiguratio - the Controller annotation name made more sense on the ResourceController. What it really does it provides configuration values for the controller (not really to the Reconciler), ideally it would have the name @ControllerConfiguration but we already have an interface elsewhere with that name. So it is still open how to rename this annotation if rename at all.

  4. EventSourceManager to EventSourceRegistry - see below.

Internal Changes:

  1. ConfiguredController -> Controller - after also a bit of refactoring the ConfiguredController is a high level aggregate that mostly directly (some indirectly) encapsulates all the classes regarding to one registered controller. Also now that the ResourceController is renamed to Reconciler the ConfiguredController with it's original meaning does not make that much sense. Note that also after refactoring the controller is responsible of starting and stoping most of the major components from one place, what is now much cleaner.

  2. DefaultEventHandler -> EventProcessor - We have an interface called EventHandler which is realted to EventSources, thus where the event source sends the events. The DefaultEventHandler is processing the event, but we received more comments, why is there a DefaultEventHandler is this class clearly cannot be substituted with a different implementation. So renamed DefaultEventHandler to make it more explicit that the intention to have a EventHandler interface is not to make it possibly replaceable with different implementation, but to segregate the interface what is facing the user (who implements an EventSource )

  3. EventSourceManager to EventSourceRegistry AND DefaultEventSourceManager -> EventSourceManager - similar as in previous, the purpose of EventSourceManager was to have a separate interface facing the user who registers a event sources. So it makes sense made sense to rename it to EventSourceRegistry, and to make it more clear that the intention is not to give possibility to substitute DefaultEventSourceManager with other implementation, it was renamed to EventSourceManager

  4. EventDispatcher -> ReconciliationDispatcher - the dispatching is not related at all anymore to events. Dispatching meant in this context, which method if the Reconciler is called. This is based on the actual values in the custom resource.
    This has also other responsibilities (like executing the the updates on custom resource, instructed by update control), so in case this name can be further refined.

v1 Model:
v1_model

v2 Model:
v2_model

see also:
#615
#374

@csviri csviri added this to the v2 milestone Nov 4, 2021
@csviri csviri self-assigned this Nov 4, 2021
@csviri csviri linked a pull request Nov 5, 2021 that will close this issue
@csviri csviri changed the title Summary of Proposed changes for Naming and related Refactoring for V2 Summary of Proposed Changes for Naming and Related Refactoring for V2 Nov 5, 2021
@adam-sandor
Copy link
Collaborator

I really like those changes! Reconciler FTW :)

@lburgazzoli
Copy link
Collaborator

+1 with me

@csviri
Copy link
Collaborator Author

csviri commented Nov 8, 2021

@Controller renamed to @ControllerConfiguration at the end, since it's in different package then the ControllerConfiguration interface, it should be that confusing maybe not at all that we have an annotation and an interface. Actually, the 2 are related, basically filling the controller configurations from the annotation.

@heesuk-ahn
Copy link
Contributor

I think The V2 model appears to be a more intuitive and understandable architectural model. 👍

csviri added a commit that referenced this issue Nov 10, 2021
Renaming and refactoring structures based on the rational described in #655
@csviri csviri closed this as completed Nov 10, 2021
@metacosm metacosm pinned this issue Nov 15, 2021
@csviri csviri unpinned this issue Apr 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants