Question & Opinion) ResourceController's init(EventSourceManager) method feels a bit vague.

[heesuk-ahn]

HI, all
I was wondering how to register eventSource in Controller, so I looked at the code.

https://github.com/java-operator-sdk/java-operator-sdk/blob/2e8f219437e53b9ca58f146a24397dbf38f0bfb8/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java#L55

Looking at this method, it looks like I can create and use my own EventSourceManager.

However, in reality, when operator.start() starts, DefaultEventSourceManager is unconditionally injected in configuredController.
(https://github.com/java-operator-sdk/java-operator-sdk/blob/2e8f219437e53b9ca58f146a24397dbf38f0bfb8/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java#L178-L179)

And actually, the user should directly register the EventSource that the user wants to register with the EventSourceManager inside the init method.
like this

  @Override
  public void init(EventSourceManager eventSourceManager) {
    ...
    eventSourceManager.registerEventSource(eventSource);
  }

Personally, I think this might be a bit ambiguous.
It feels like the user can directly inject and use CustomEventSourceManager .

If that's not what you intended, Rather, I think the interface below would be a little clearer.

public interface ResourceController<R extends CustomResource> {
 ...
  default List<EventSource> registerEventSources() {
    return List.of();
  }
}

And by calling this method in the framework, it will be able to directly register it with the event source manager.

Since EventSourceManager is an important component in the framework, it seems to be able to prevent it from being directly exposed to users.

This is just my personal opinion.
Thank you for reading this question :)

[metacosm]

Hi @heesuk-ahn, I agree with you that the intent of the init method is a little unclear. While it indeed is mostly used to register event sources, it also provides a way for your controller to hold onto the EventSourceManager so that it could be queried. To be fair, I'm not sure how common of a use case that might be. Your idea of inverting the intent i.e. to let the controller provide the list of event sources to register instead of passing the manager for the controller to do the registration itself is interesting and something I think we should consider doing.

[csviri]

@heesuk-ahn thx for the feedback!!
I agree too. Linked a PR, which is just a minor improvement on the current approach. The thing is that historically this was a pre-hook to manage possible other initializations before any reconciliation happening. As @metacosm mentioned already, mostly only makes sense for registering event sources AND getting access for CustomResourceEventSource.

To have access to this class is essential, to get access to the caches related to the custom resource, if we could separate/segragate that maybe access some abstracted access in the context for a cache interface? Then this might make sense.

default` List<EventSource> registerEventSources() {
    return List.of();
}

[lburgazzoli]

Looking at the architecture diagram of Kubebuilder https://book.kubebuilder.io/architecture.html, there is a distinction between the Controller and the Reconciler, in the java operator sdk we are conflating the two concept in a single ResourceController interface however I wonder if we should reason about having a clear split between the two concept which IMHO, could make it easier to split the responsibility and to keep a clean interface so:

• Controller : where events source and ilter are registered, and where advanced set-up may happen
• Reconciler : business logic

Creating a controller should not be required so a minimal operator could still be implemented by providing the implementation of a Reconciler. If any advanced set-up is required, then a Controller can be provided i.e:

var operator = new Operator();
operator.register(new MyReconciler(), new MyController())

@Reconciler(controller = MyController.class) 
class MyReconciler {
}

[csviri]

@lburgazzoli I have quite a mixed feelings following this approach. But will dig deeper in the go impl since not that familiar with this part.

In addition to that this segregation does are not really present in the domain language itself. I agree that it might be nice to have a separate place where we can more complex configuration. Although don't think that the current way is particularly ugly.

Also denoted on that architecture diagram, this is quite fundamentally different approach, in our case its that controller seems to be more closer to DefaulEventHandler where the complex execution logic happening.

But excellent point that we should start a discussion around that.

[metacosm]

If we were to go down that line, I think I'd rather put the init method on a separate interface that Controllers could implement if needed to add more advanced behavior.
That said, I think that some of these issues will go away once we start working more closely on dependent resources.

[lburgazzoli]

Also denoted on that architecture diagram, this is quite fundamentally different approach, in our case its that controller seems to be more closer to DefaulEventHandler where the complex execution logic happening.

Maybe we should just rename DefaulEventHandler to Controller :)

[lburgazzoli]

If we were to go down that line, I think I'd rather put the init method on a separate interface that Controllers could implement if needed to add more advanced behavior.

That would be ok.

My main point here is that the boundaries of the ResourceController as today are a little bit blurred and sometimes confusing as example, you can configure some aspects of the controller using annotations or the configuration object and something else with the init method so I think it would be nice if everything could be configured programmatically in one place (I admit I have not yet had a look at the recent changes)

[csviri]

Will update the PR with a segregated interface.

[csviri]

Updated the PR, pls suggest better naming in case :)