Skip to content

fix: EventSourceManager and ResourceController interface enhancements #618

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.

Sign up for GitHub

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 2 commits into from
Oct 26, 2021
Merged

fix: EventSourceManager and ResourceController interface enhancements #618

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7a3c698
fix: EventSourceManager and ResourceController interface enhancements
csviri Oct 25, 2021
cd02f34
fix: segration of EventSource initialization
csviri Oct 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions ...-framework-core/src/main/java/io/javaoperatorsdk/operator/api/EventSourceInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
package io.javaoperatorsdk.operator.api;

import io.fabric8.kubernetes.client.CustomResource;
import io.javaoperatorsdk.operator.processing.event.EventSourceManager;

public interface EventSourceInitializer<T extends CustomResource<?, ?>> {

/**
* In this typically you might want to register event sources. But can access
* CustomResourceEventSource, what might be handy for some edge cases.
*
* @param eventSourceManager the {@link EventSourceManager} where event sources can be registered.
*/
void prepareEventSources(EventSourceManager<T> eventSourceManager);

}
8 changes: 0 additions & 8 deletions ...ator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/ResourceController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
package io.javaoperatorsdk.operator.api;

import io.fabric8.kubernetes.client.CustomResource;
import io.javaoperatorsdk.operator.processing.event.EventSourceManager;

public interface ResourceController<R extends CustomResource> {

Expand Down Expand Up @@ -49,11 +48,4 @@ default DeleteControl deleteResource(R resource, Context<R> context) {
*/
UpdateControl<R> createOrUpdateResource(R resource, Context<R> context);

/**
* In init typically you might want to register event sources.
*
* @param eventSourceManager the {@link EventSourceManager} which handles this controller and with
* which event sources can be registered
*/
default void init(EventSourceManager eventSourceManager) {}
}
15 changes: 7 additions & 8 deletions ...ework-core/src/main/java/io/javaoperatorsdk/operator/processing/ConfiguredController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,20 +14,17 @@
import io.javaoperatorsdk.operator.Metrics.ControllerExecution;
import io.javaoperatorsdk.operator.MissingCRDException;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.api.Context;
import io.javaoperatorsdk.operator.api.DeleteControl;
import io.javaoperatorsdk.operator.api.ResourceController;
import io.javaoperatorsdk.operator.api.UpdateControl;
import io.javaoperatorsdk.operator.api.*;
import io.javaoperatorsdk.operator.api.config.ControllerConfiguration;
import io.javaoperatorsdk.operator.processing.event.DefaultEventSourceManager;
import io.javaoperatorsdk.operator.processing.event.EventSourceManager;

public class ConfiguredController<R extends CustomResource<?, ?>> implements ResourceController<R>,
Closeable {
Closeable, EventSourceInitializer {
private final ResourceController<R> controller;
private final ControllerConfiguration<R> configuration;
private final KubernetesClient kubernetesClient;
private EventSourceManager eventSourceManager;
private DefaultEventSourceManager eventSourceManager;

public ConfiguredController(ResourceController<R> controller,
ControllerConfiguration<R> configuration,
Expand Down Expand Up @@ -97,7 +94,7 @@ public UpdateControl<R> execute() {
}

@Override
public void init(EventSourceManager eventSourceManager) {
public void prepareEventSources(EventSourceManager eventSourceManager) {
throw new UnsupportedOperationException("This method should never be called directly");
}

Expand Down Expand Up @@ -169,7 +166,9 @@ public void start() throws OperatorException {

try {
eventSourceManager = new DefaultEventSourceManager<>(this);
controller.init(eventSourceManager);
if (controller instanceof EventSourceInitializer) {
((EventSourceInitializer) controller).prepareEventSources(eventSourceManager);
}
} catch (MissingCRDException e) {
throwMissingCRDException(crdName, specVersion, controllerName);
}
Expand Down
3 changes: 2 additions & 1 deletion ...src/main/java/io/javaoperatorsdk/operator/processing/event/DefaultEventSourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package io.javaoperatorsdk.operator.processing.event;

import java.io.Closeable;
import java.util.*;
import java.util.concurrent.locks.ReentrantLock;

Expand All @@ -15,7 +16,7 @@
import io.javaoperatorsdk.operator.processing.event.internal.TimerEventSource;

public class DefaultEventSourceManager<R extends CustomResource<?, ?>>
implements EventSourceManager {
implements EventSourceManager<R>, Closeable {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why move Closeable to the implementation? Shouldn't all EventSourceManager implementations be Closeable?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

EventSourceManager is just the API that user should see from the init method. Should not be aware that it is actually closeable, it's an implementation detail in the background.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, if we had several implementations of the interface, all of them should be closeable. So, to me, Closeable should be part of the contract of the EventSourceManager interface. Otherwise, if we push your logic to its conclusion, then we shouldn't even have a interface because we only have one implementation and that implementation needs to be closeable for the SDK to work properly. 😄
If we don't expect any other implementations, I don't think we need to worry about having an interface just for the sake of hiding some things away. Worse, having an interface implies that we potentially expect several implementations, which isn't really the case. It makes things more complex without much benefit.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it makes sense, it's basically interface segregation principle:
https://en.wikipedia.org/wiki/Interface_segregation_principle
The intended users should not see the other methods if we don't intend it for them.
They should not care if there is an other implementation or not, this is what we provide, it should be used.

The benefit is clear user facing interface. But without to much added complexity (close to 0).

Copy link
Collaborator Author

@csviri csviri Oct 26, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

maybe @lburgazzoli or @adam-sandor , what do you think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

discussed with @metacosm . Will merge it now, and will do subsequent PRs, if we find a better structure/way.


private static final Logger log = LoggerFactory.getLogger(DefaultEventSourceManager.class);

Expand Down
6 changes: 1 addition & 5 deletions ...k-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,14 +1,12 @@
package io.javaoperatorsdk.operator.processing.event;

import java.io.Closeable;
import java.io.IOException;
import java.util.Set;

import io.fabric8.kubernetes.client.CustomResource;
import io.javaoperatorsdk.operator.OperatorException;
import io.javaoperatorsdk.operator.processing.event.internal.CustomResourceEventSource;

public interface EventSourceManager<T extends CustomResource<?, ?>> extends Closeable {
public interface EventSourceManager<T extends CustomResource<?, ?>> {

/**
* Add the {@link EventSource} identified by the given <code>name</code> to the event manager.
Expand All @@ -25,6 +23,4 @@ void registerEventSource(EventSource eventSource)

CustomResourceEventSource<T> getCustomResourceEventSource();

@Override
default void close() throws IOException {}
}
10 changes: 4 additions & 6 deletions ...ava/io/javaoperatorsdk/operator/sample/event/EventSourceTestCustomResourceController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,17 +7,15 @@

import io.fabric8.kubernetes.client.CustomResource;
import io.javaoperatorsdk.operator.ControllerUtils;
import io.javaoperatorsdk.operator.api.Context;
import io.javaoperatorsdk.operator.api.Controller;
import io.javaoperatorsdk.operator.api.ResourceController;
import io.javaoperatorsdk.operator.api.UpdateControl;
import io.javaoperatorsdk.operator.api.*;
import io.javaoperatorsdk.operator.processing.event.EventSourceManager;
import io.javaoperatorsdk.operator.processing.event.internal.TimerEventSource;
import io.javaoperatorsdk.operator.support.TestExecutionInfoProvider;

@Controller
public class EventSourceTestCustomResourceController
implements ResourceController<EventSourceTestCustomResource>, TestExecutionInfoProvider {
implements ResourceController<EventSourceTestCustomResource>, EventSourceInitializer,
TestExecutionInfoProvider {

public static final String FINALIZER_NAME =
ControllerUtils.getDefaultFinalizerName(
Expand All @@ -31,7 +29,7 @@ public class EventSourceTestCustomResourceController
new TimerEventSource<>();

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

Expand Down
10 changes: 4 additions & 6 deletions .../operator/sample/informereventsource/InformerEventSourceTestCustomResourceController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,7 @@

import io.fabric8.kubernetes.api.model.ConfigMap;
import io.fabric8.kubernetes.client.KubernetesClient;
import io.javaoperatorsdk.operator.api.Context;
import io.javaoperatorsdk.operator.api.Controller;
import io.javaoperatorsdk.operator.api.ResourceController;
import io.javaoperatorsdk.operator.api.UpdateControl;
import io.javaoperatorsdk.operator.api.*;
import io.javaoperatorsdk.operator.junit.KubernetesClientAware;
import io.javaoperatorsdk.operator.processing.event.EventSourceManager;
import io.javaoperatorsdk.operator.processing.event.internal.InformerEventSource;
Expand All @@ -22,7 +19,8 @@
*/
@Controller(finalizerName = NO_FINALIZER)
public class InformerEventSourceTestCustomResourceController implements
ResourceController<InformerEventSourceTestCustomResource>, KubernetesClientAware {
ResourceController<InformerEventSourceTestCustomResource>, KubernetesClientAware,
EventSourceInitializer {

private static final Logger LOGGER =
LoggerFactory.getLogger(InformerEventSourceTestCustomResourceController.class);
Expand All @@ -34,7 +32,7 @@ public class InformerEventSourceTestCustomResourceController implements
private InformerEventSource<ConfigMap> eventSource;

@Override
public void init(EventSourceManager eventSourceManager) {
public void prepareEventSources(EventSourceManager eventSourceManager) {
eventSource = new InformerEventSource<>(kubernetesClient, ConfigMap.class,
Mappers.fromAnnotation(RELATED_RESOURCE_UID));
eventSourceManager.registerEventSource(eventSource);
Expand Down