From 2a44fb0f067067e777a60673230b991c21690f84 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 25 May 2022 15:16:53 +0200 Subject: [PATCH 1/3] fix: refactor event source holder --- .../processing/event/EventSourceManager.java | 14 ++++++++------ .../operator/processing/event/EventSources.java | 17 +++++++++++------ .../processing/event/EventSourcesTest.java | 6 +++--- 3 files changed, 22 insertions(+), 15 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java index 3990c8c300..29df2c6b8a 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java @@ -39,13 +39,15 @@ public EventSourceManager(Controller controller) { this.eventSources = eventSources; this.controller = controller; // controller event source needs to be available before we create the event processor - final var controllerEventSource = eventSources.initControllerEventSource(controller); + eventSources.initControllerEventSource(controller); this.eventProcessor = new EventProcessor<>(this); - // sources need to be registered after the event processor is created since it's set on the - // event source - registerEventSource(eventSources.retryEventSource()); - registerEventSource(controllerEventSource); + postProcessDefaultEventSources(); + } + + private void postProcessDefaultEventSources() { + eventSources.controllerResourceEventSource().setEventHandler(eventProcessor); + eventSources.retryEventSource().setEventHandler(eventProcessor); } /** @@ -146,7 +148,7 @@ public void broadcastOnResourceEvent(ResourceAction action, R resource, R oldRes public void changeNamespaces(Set namespaces) { eventProcessor.stop(); eventSources - .allEventSources() + .eventSources() .filter(NamespaceChangeable.class::isInstance) .map(NamespaceChangeable.class::cast) .filter(NamespaceChangeable::allowsNamespaceChanges) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java index 42f0638968..3dd41331d4 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java @@ -16,7 +16,11 @@ class EventSources implements Iterable { - private static final String CONTROLLER_EVENT_SOURCE_KEY = "0"; + public static final String CONTROLLER_RESOURCE_EVENT_SOURCE_NAME = + "ControllerResourceEventSource"; + public static final String RETRY_RESCHEDULE_TIMER_EVENT_SOURCE_NAME = + "RetryAndRescheduleTimerEventSource"; + private final ConcurrentNavigableMap> sources = new ConcurrentSkipListMap<>(); private final TimerEventSource retryAndRescheduleTimerEventSource = new TimerEventSource<>(); @@ -38,7 +42,11 @@ TimerEventSource retryEventSource() { @Override public Iterator iterator() { - return flatMappedSources().iterator(); + return Stream.concat(Stream.of( + new NamedEventSource(controllerResourceEventSource, CONTROLLER_RESOURCE_EVENT_SOURCE_NAME), + new NamedEventSource(retryAndRescheduleTimerEventSource, + RETRY_RESCHEDULE_TIMER_EVENT_SOURCE_NAME)), + flatMappedSources()).iterator(); } Stream flatMappedSources() { @@ -46,7 +54,7 @@ Stream flatMappedSources() { .map(esEntry -> new NamedEventSource(esEntry.getValue(), esEntry.getKey()))); } - Stream allEventSources() { + Stream eventSources() { return Stream.concat( Stream.of(retryEventSource(), controllerResourceEventSource()).filter(Objects::nonNull), sources.values().stream().flatMap(c -> c.values().stream())); @@ -81,9 +89,6 @@ private Class getResourceType(EventSource source) { } private String keyFor(EventSource source) { - if (source instanceof ControllerResourceEventSource) { - return CONTROLLER_EVENT_SOURCE_KEY; - } return keyFor(getResourceType(source)); } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java index 3bc223653f..0393680110 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java @@ -31,7 +31,7 @@ void cannotAddTwoEventSourcesWithSameName() { @Test void allEventSourcesShouldReturnAll() { // initial state doesn't have ControllerResourceEventSource - assertEquals(Set.of(eventSources.retryEventSource()), eventSources.allEventSources().collect( + assertEquals(Set.of(eventSources.retryEventSource()), eventSources.eventSources().collect( Collectors.toSet())); final var configuration = MockControllerConfiguration.forResource(HasMetadata.class); final var controller = new Controller(mock(Reconciler.class), configuration, @@ -39,12 +39,12 @@ void allEventSourcesShouldReturnAll() { eventSources.initControllerEventSource(controller); assertEquals( Set.of(eventSources.retryEventSource(), eventSources.controllerResourceEventSource()), - eventSources.allEventSources().collect(Collectors.toSet())); + eventSources.eventSources().collect(Collectors.toSet())); final var source = mock(EventSource.class); eventSources.add("foo", source); assertEquals(Set.of(eventSources.retryEventSource(), eventSources.controllerResourceEventSource(), source), - eventSources.allEventSources().collect(Collectors.toSet())); + eventSources.eventSources().collect(Collectors.toSet())); } } From 58d2ea32cad4144aa63e3836e616a23c2fa911f0 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 25 May 2022 15:30:57 +0200 Subject: [PATCH 2/3] added unit tests --- .../processing/event/EventSources.java | 2 +- .../processing/event/NamedEventSource.java | 17 ++++++++ .../processing/event/EventSourcesTest.java | 42 +++++++++++++++---- 3 files changed, 52 insertions(+), 9 deletions(-) diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java index 3dd41331d4..aebe6caecb 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java @@ -56,7 +56,7 @@ Stream flatMappedSources() { Stream eventSources() { return Stream.concat( - Stream.of(retryEventSource(), controllerResourceEventSource()).filter(Objects::nonNull), + Stream.of(controllerResourceEventSource(), retryEventSource()).filter(Objects::nonNull), sources.values().stream().flatMap(c -> c.values().stream())); } diff --git a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/NamedEventSource.java b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/NamedEventSource.java index 38d262f254..92d1f40096 100644 --- a/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/NamedEventSource.java +++ b/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/NamedEventSource.java @@ -1,5 +1,7 @@ package io.javaoperatorsdk.operator.processing.event; +import java.util.Objects; + import io.javaoperatorsdk.operator.OperatorException; import io.javaoperatorsdk.operator.processing.event.source.EventSource; @@ -40,4 +42,19 @@ public String toString() { public EventSource original() { return original; } + + @Override + public boolean equals(Object o) { + if (this == o) + return true; + if (o == null || getClass() != o.getClass()) + return false; + NamedEventSource that = (NamedEventSource) o; + return Objects.equals(original, that.original) && Objects.equals(name, that.name); + } + + @Override + public int hashCode() { + return Objects.hash(original, name); + } } diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java index 0393680110..238122c289 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java @@ -12,12 +12,16 @@ import io.javaoperatorsdk.operator.processing.Controller; import io.javaoperatorsdk.operator.processing.event.source.EventSource; +import static io.javaoperatorsdk.operator.processing.event.EventSources.CONTROLLER_RESOURCE_EVENT_SOURCE_NAME; +import static io.javaoperatorsdk.operator.processing.event.EventSources.RETRY_RESCHEDULE_TIMER_EVENT_SOURCE_NAME; +import static org.assertj.core.api.Assertions.assertThat; import static org.junit.jupiter.api.Assertions.*; import static org.mockito.Mockito.mock; @SuppressWarnings({"unchecked", "rawtypes"}) class EventSourcesTest { + public static final String EVENT_SOURCE_NAME = "foo"; EventSources eventSources = new EventSources(); @Test @@ -33,18 +37,40 @@ void allEventSourcesShouldReturnAll() { // initial state doesn't have ControllerResourceEventSource assertEquals(Set.of(eventSources.retryEventSource()), eventSources.eventSources().collect( Collectors.toSet())); + + initControllerEventSource(); + + assertThat(eventSources.eventSources()).containsExactly( + eventSources.controllerResourceEventSource(), + eventSources.retryEventSource()); + + final var source = mock(EventSource.class); + eventSources.add(EVENT_SOURCE_NAME, source); + // order matters + assertThat(eventSources.eventSources()) + .containsExactly(eventSources.controllerResourceEventSource(), + eventSources.retryEventSource(), source); + } + + @Test + void eventSourcesIteratorShouldReturnControllerEventSourceAsFirst() { + initControllerEventSource(); + final var source = mock(EventSource.class); + eventSources.add(EVENT_SOURCE_NAME, source); + + assertThat(eventSources.iterator()).toIterable().containsExactly( + new NamedEventSource(eventSources.controllerResourceEventSource(), + CONTROLLER_RESOURCE_EVENT_SOURCE_NAME), + new NamedEventSource(eventSources.retryEventSource(), + RETRY_RESCHEDULE_TIMER_EVENT_SOURCE_NAME), + new NamedEventSource(source, EVENT_SOURCE_NAME)); + } + + private void initControllerEventSource() { final var configuration = MockControllerConfiguration.forResource(HasMetadata.class); final var controller = new Controller(mock(Reconciler.class), configuration, MockKubernetesClient.client(HasMetadata.class)); eventSources.initControllerEventSource(controller); - assertEquals( - Set.of(eventSources.retryEventSource(), eventSources.controllerResourceEventSource()), - eventSources.eventSources().collect(Collectors.toSet())); - final var source = mock(EventSource.class); - eventSources.add("foo", source); - assertEquals(Set.of(eventSources.retryEventSource(), - eventSources.controllerResourceEventSource(), source), - eventSources.eventSources().collect(Collectors.toSet())); } } From 27c53e23cfdce8bb498b92a0a7fce6a9ecb13d29 Mon Sep 17 00:00:00 2001 From: csviri Date: Wed, 25 May 2022 16:01:46 +0200 Subject: [PATCH 3/3] improved unit test --- .../operator/processing/event/EventSourcesTest.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java index 238122c289..ecc5c12079 100644 --- a/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java +++ b/operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java @@ -1,8 +1,5 @@ package io.javaoperatorsdk.operator.processing.event; -import java.util.Set; -import java.util.stream.Collectors; - import org.junit.jupiter.api.Test; import io.fabric8.kubernetes.api.model.HasMetadata; @@ -35,8 +32,7 @@ void cannotAddTwoEventSourcesWithSameName() { @Test void allEventSourcesShouldReturnAll() { // initial state doesn't have ControllerResourceEventSource - assertEquals(Set.of(eventSources.retryEventSource()), eventSources.eventSources().collect( - Collectors.toSet())); + assertThat(eventSources.eventSources()).containsExactly(eventSources.retryEventSource()); initControllerEventSource();