Skip to content

refactor: renaming core classes and APIs #646

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

Merged
merged 27 commits into from
Nov 10, 2021
Merged

refactor: renaming core classes and APIs #646

merged 27 commits into from
Nov 10, 2021

Conversation

csviri
Copy link
Collaborator

@csviri csviri commented Nov 2, 2021

Considering this as the next step for refactoring the core classes. Not necessary final in this regard, we might want to do more iterations on this.

@csviri csviri marked this pull request as draft November 2, 2021 09:47
@csviri csviri marked this pull request as ready for review November 3, 2021 08:04
@csviri csviri linked an issue Nov 3, 2021 that may be closed by this pull request
@csviri csviri changed the title refactor: renaming central classes and APIs refactor: renaming core classes and APIs Nov 3, 2021
@@ -126,28 +126,28 @@ public void close() {
* passing it the controller's original configuration. The effective registration of the
* controller is delayed till the operator is started.
*
* @param controller the controller to register
* @param reconciler part of the controller to register
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think that if we do this change, we need to remove the controller part completely everywhere and only talk about Reconciler because otherwiser it gets too confusing. Like what is a Controller is our SDK?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Answered above, the controller. Reconciler is just the logic how to reconciler, but this is basically same in controller runtime, the controller is the whole , reconciler is just part of it.
Unfortunate that we don't have a class representing the controller itslef, Maybe to renamae ConfiguredController to Controller?

import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager;
import io.javaoperatorsdk.operator.processing.event.EventSourceManager;

public class ConfiguredController<R extends CustomResource<?, ?>> implements ResourceController<R>,
public class ConfiguredController<R extends CustomResource<?, ?>> implements Reconciler<R>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe this class should be renamed ConfiguredReconciler or maybe simply Controller, where for our SDK a Controller would be a Reconciler associated with its configuration? In this case, we should also rename the Controller annotation to something different, maybe Configuration… Then again, the more I think about it, maybe we shouldn't be renaming things too much if there is no obvious benefit from doing so… because otherwise it will mean that we confuse our existing users and we will need to update all our existing documentation / articles…

Copy link
Collaborator Author

@csviri csviri Nov 4, 2021

Choose a reason for hiding this comment

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

hehe, yes also wrote this above , maybe this should be the Controller, and the @Controller annotation should be @ControllerConfiguration.

I think this class should be our high level aggregate (therefore name it as Controller), so it will contiain the Reconciler, the EventSourceManager, the EventProcessor (former DefaultEventHandler). Basically every aspect of a single controller instance.

The benefit is that we are now closer to go terminology, but on the other hand yes it can be confusing for the users. But getting it right now for v2 is probably the good time to do it "once and for all" :)

- ConfiguredController now feels more like an overall aggregate. Now centrally starts and stop some aggregated components.
@csviri csviri requested a review from metacosm November 4, 2021 13:56
# Conflicts:
#	operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomReconciler.java
@@ -13,7 +13,7 @@
public interface ControllerConfiguration<R extends CustomResource<?, ?>> {

default String getName() {
return ControllerUtils.getDefaultResourceReconcilerName(getAssociatedControllerClassName());
return ControllerUtils.getDefaultReconcilerName(getAssociatedControllerClassName());
Copy link
Collaborator

Choose a reason for hiding this comment

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

getAssociatedControllerClassName should also be renamed to getAssociatedReconcilerClassName

@csviri csviri linked an issue Nov 5, 2021 that may be closed by this pull request
DECISION_LOG.md Outdated
@@ -7,6 +7,6 @@

The original idea was to explicitly support retry in the scheduler. However, this turned out to complicate the algorithm
in case of event sources. Mostly it would be harder to manage the buffer, and the other event sources, thus what
does it mean for other event sources that there was a failed controller execution? Probably it would be better to
manage this in an abstract controller, and just use the "reprocess event-source" source in case of an error.
does it mean for other event sources that there was a failed controllerConfiguration execution? Probably it would be better to
Copy link
Collaborator

Choose a reason for hiding this comment

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

Looks like you got too eager with search and replace :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's intellij too intelligent :) will fix these

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

but probably we can delete Decision log since it is not used

README.md Outdated
@@ -130,7 +130,7 @@ dependencies {
```

Once you've added the dependency, define a main method initializing the Operator and registering a
controller.
controllerConfiguration.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

@@ -14,7 +14,7 @@ other configuration options are provided to fine tune or turn off these features
## Controller Execution in a Nutshell

Controller execution is always triggered by an event. Events typically come from the custom resource
(i.e. custom resource is created, updated or deleted) that the controller is watching, but also from different sources
(i.e. custom resource is created, updated or deleted) that the controllerConfiguration is watching, but also from different sources
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

@@ -24,7 +24,7 @@ i.e. [ResourceController](https://github.com/java-operator-sdk/java-operator-sdk
called, a post-processing phase follows, where typically framework checks if:

- an exception was thrown during execution, if yes schedules a retry.
- there are new events received during the controller execution, if yes schedule the execution again.
- there are new events received during the controllerConfiguration execution, if yes schedule the execution again.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same.

Copy link
Collaborator

@metacosm metacosm left a comment

Choose a reason for hiding this comment

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

Many instances of controller got renamed to controllerConfiguration when they shouldn't have.

@csviri
Copy link
Collaborator Author

csviri commented Nov 8, 2021

Many instances of controller got renamed to controllerConfiguration when they shouldn't have.

ahh ok thx, will check it.

@csviri
Copy link
Collaborator Author

csviri commented Nov 8, 2021

@metacosm @lburgazzoli @adam-sandor can I merge this?
will address the stability issues of tests in a separate PR.

@csviri csviri merged commit 86245ca into v2 Nov 10, 2021
@csviri csviri deleted the renamings-reconciler branch November 10, 2021 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants