8000 fix: event sources no ordered start & missing event source throws exc… · rsynek/java-operator-sdk@2eac098 · GitHub
[go: up one dir, main page]
Skip to content

Commit 2eac098

Browse files
csviricsviri
authored
fix: event sources no ordered start & missing event source throws exception (operator-framework#1104)
1 parent 9900030 commit 2eac098

File tree

15 files changed

+101
-86
lines changed

15 files changed

+101
-86
lines changed

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/DefaultContext.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -32,7 +32,7 @@ public Optional<RetryInfo> getRetryInfo() {
3232
public <T> Optional<T> getSecondaryResource(Class<T> expectedType, String eventSourceName) {
3333< B41A /code>
return controller.getEventSourceManager()
3434
.getResourceEventSourceFor(expectedType, eventSourceName)
35-
.flatMap(es -> es.getSecondaryResource(primaryResource));
35+
.getSecondaryResource(primaryResource);
3636
}
3737

3838
@Override

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/api/reconciler/EventSourceInitializer.java

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -25,17 +25,30 @@ public interface EventSourceInitializer<P extends HasMetadata> {
2525
Map<String, EventSource> prepareEventSources(EventSourceContext<P> context);
2626

2727
/**
28-
* Utility method to easily create map with default names of event sources.
28+
* Utility method to easily create map with generated name for event sources. This is for the use
29+
* case when the event sources are not access explicitly by name in the reconciler.
2930
*
3031
* @param eventSources to name
3132
* @return even source with default names
3233
*/
33-
static Map<String, EventSource> defaultNamedEventSources(EventSource... eventSources) {
34+
static Map<String, EventSource> nameEventSources(EventSource... eventSources) {
3435
Map<String, EventSource> eventSourceMap = new HashMap<>(eventSources.length);
3536
for (EventSource eventSource : eventSources) {
36-
eventSourceMap.put(EventSource.defaultNameFor(eventSource), eventSource);
37+
eventSourceMap.put(generateNameFor(eventSource), eventSource);
3738
}
3839
return eventSourceMap;
3940
}
4041

42+
/**
43+
* This is for the use case when the event sources are not access explicitly by name in the
44+
* reconciler.
45+
*
46+
* @param eventSource EventSource
47+
* @return generated name
48+
*/
49+
static String generateNameFor(EventSource eventSource) {
50+
// we can have multiple event sources for the same class
51+
return eventSource.getClass().getName() + "#" + eventSource.hashCode();
52+
}
53+
4154
}

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSourceManager.java

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22

33
import java.util.LinkedHashSet;
44
import java.util.Objects;
5-
import java.util.Optional;
65
import java.util.Set;
76
import java.util.concurrent.locks.ReentrantLock;
87
import java.util.stream.Collectors;
@@ -13,6 +12,7 @@
1312
import io.fabric8.kubernetes.api.model.HasMetadata;
1413
import io.javaoperatorsdk.operator.MissingCRDException;
1514
import io.javaoperatorsdk.operator.OperatorException;
15+
import io.javaoperatorsdk.operator.api.reconciler.EventSourceInitializer;
1616
import io.javaoperatorsdk.operator.processing.Controller;
1717
import io.javaoperatorsdk.operator.processing.LifecycleAware;
1818
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
@@ -123,7 +123,7 @@ public final void registerEventSource(String name, EventSource eventSource)
123123
lock.lock();
124124
try {
125125
if (name == null || name.isBlank()) {
126-
name = EventSource.defaultNameFor(eventSource);
126+
name = EventSourceInitializer.generateNameFor(eventSource);
127127
}
128128
eventSources.add(name, eventSource);
129129
eventSource.setEventHandler(eventProcessor);
@@ -171,19 +171,15 @@ public ControllerResourceEventSource<R> getControllerResourceEventSource() {
171171
return eventSources.controllerResourceEventSource();
172172
}
173173

174-
public <S> Optional<ResourceEventSource<S, R>> getResourceEventSourceFor(
174+
<S> ResourceEventSource<S, R> getResourceEventSourceFor(
175175
Class<S> dependentType) {
176176
return getResourceEventSourceFor(dependentType, null);
177177
}
178178

179-
public <S> Optional<ResourceEventSource<S, R>> getResourceEventSourceFor(
179+
public <S> ResourceEventSource<S, R> getResourceEventSourceFor(
180180
Class<S> dependentType, String qualifier) {
181-
if (dependentType == null) {
182-
return Optional.empty();
183-
}
184-
String name = qualifier == null ? "" : qualifier;
185-
final var eventSource = eventSources.get(dependentType, name);
186-
return Optional.ofNullable(eventSource);
181+
Objects.requireNonNull(dependentType, "dependentType is Mandatory");
182+
return eventSources.get(dependentType, qualifier);
187183
}
188184

189185
TimerEventSource<R> retryEventSource() {

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/EventSources.java

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -61,48 +61,38 @@ public boolean contains(String name, EventSource source) {
6161
public void add(String name, EventSource eventSource) {
6262
if (contains(name, eventSource)) {
6363
throw new IllegalArgumentException("An event source is already registered for the "
64-
+ keyAsString(getDependentType(eventSource), name)
64+
+ keyAsString(getResourceType(eventSource), name)
6565
+ " class/name combination");
6666
}
6767
sources.computeIfAbsent(keyFor(eventSource), k -> new HashMap<>()).put(name, eventSource);
6868
}
6969

7070
@SuppressWarnings("rawtypes")
71-
private Class<?> getDependentType(EventSource source) {
71+
private Class<?> getResourceType(EventSource source) {
7272
return source instanceof ResourceOwner
7373
? ((ResourceOwner) source).resourceType()
7474
: source.getClass();
7575
}
7676

7777
private String keyFor(EventSource source) {
78-
return keyFor(getDependentType(source));
78+
return keyFor(getResourceType(source));
7979
}
8080

8181
private String keyFor(Class<?> dependentType) {
82-
var key = dependentType.getCanonicalName();
83-
84-
// make sure timer event source is started first, then controller event source
85-
// this is needed so that these sources are set when informer sources start so that events can
86-
// properly be processed
87-
if (controllerResourceEventSource != null
88-
&& key.equals(controllerResourceEventSource.resourceType().getCanonicalName())) {
89-
key = 1 + "-" + key;
90-
} else if (key.equals(retryAndRescheduleTimerEventSource.getClass().getCanonicalName())) {
91-
key = 0 + "-" + key;
92-
}
93-
return key;
82+
return dependentType.getCanonicalName();
9483
}
9584

9685
@SuppressWarnings("unchecked")
9786
public <S> ResourceEventSource<S, R> get(Class<S> dependentType, String name) {
9887
final var sourcesForType = sources.get(keyFor(dependentType));
9988
if (sourcesForType == null || sourcesForType.isEmpty()) {
100-
return null;
89+
throw new IllegalArgumentException(
90+
"There is no event source found for class:" + dependentType.getName());
10191
}
10292

10393
final var size = sourcesForType.size();
10494
final EventSource source;
105-
if (size == 1) {
95+
if (size == 1 && name == null) {
10696
source = sourcesForType.values().stream().findFirst().orElse(null);
10797
} else {
10898
if (name == null || name.isBlank()) {
@@ -114,7 +104,8 @@ public <S> ResourceEventSource<S, R> get(Class<S> dependentType, String name) {
114104
source = sourcesForType.get(name);
115105

116106
if (source == null) {
117-
return null;
107+
throw new IllegalArgumentException("There is no event source found for class:" +
108+
" " + dependentType.getName() + ", name:" + name);
118109
}
119110
}
120111

‎operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/EventSource.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -20,7 +20,5 @@ public interface EventSource extends LifecycleAware {
2020
*/
2121
void setEventHandler(EventHandler handler);
2222

23-
static String defaultNameFor(EventSource source) {
24-
return source.getClass().getCanonicalName();
25-
}
23+
2624
}

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/api/reconciler/EventSourceInitializerTest.java

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
package io.javaoperatorsdk.operator.api.reconciler;
2+
3+
import java.util.HashMap;
4+
5+
import org.junit.jupiter.api.Test;
6+
7+
import io.javaoperatorsdk.operator.processing.event.source.polling.PollingEventSource;
8+
9+
import static org.assertj.core.api.Assertions.assertThat;
10+
11+
class EventSourceInitializerTest {
12+
13+
@Test
14+
void defaultNameDifferentForOtherInstance() {
15+
var eventSource1 = new PollingEventSource(() -> new HashMap<>(), 1000, String.class);
16+
var eventSource2 = new PollingEventSource(() -> new HashMap<>(), 1000, String.class);
17+
var eventSourceName1 = EventSourceInitializer.generateNameFor(eventSource1);
18+
var eventSourceName2 = EventSourceInitializer.generateNameFor(eventSource2);
19+
20+
assertThat(eventSourceName1).isNotEqualTo(eventSourceName2);
21+
}
2 10000 2+
23+
}

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java

Lines changed: 12 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,5 @@
11
package io.javaoperatorsdk.operator.processing.event;
22

3-
import java.util.Iterator;
4-
import java.util.Optional;
53
import java.util.Set;
64

75
import org.junit.jupiter.api.Test;
@@ -19,10 +17,10 @@
1917
import io.javaoperatorsdk.operator.sample.simple.TestCustomResource;
2018

2119
import static org.assertj.core.api.Assertions.assertThat;
20+
import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
2221
import static org.junit.jupiter.api.Assertions.assertEquals;
2322
import static org.junit.jupiter.api.Assertions.assertThrows;
2423
import static org.junit.jupiter.api.Assertions.assertTrue;
25-
import static org.junit.jupiter.api.Assertions.fail;
2624
import static org.mockito.Mockito.eq;
2725
import static org.mockito.Mockito.mock;
2826
import static org.mockito.Mockito.times;
@@ -76,22 +74,24 @@ public void startCascadesToEventSources() {
7674

7775
@Test
7876
void retrievingEventSourceForClassShouldWork() {
79-
assertTrue(eventSourceManager.getResourceEventSourceFor(null).isEmpty());
80-
assertTrue(eventSourceManager.getResourceEventSourceFor(Class.class).isEmpty());
77+
assertThatExceptionOfType(IllegalArgumentException.class)
78+
.isThrownBy(() -> eventSourceManager.getResourceEventSourceFor(Class.class));
8179

8280
// manager is initialized with a controller configured to handle HasMetadata
8381
EventSourceManager manager = initManager();
84-
Optional<EventSource> source = manager.getResourceEventSourceFor(HasMetadata.class);
85-
assertTrue(source.isPresent());
86-
assertTrue(source.get() instanceof ControllerResourceEventSource);
82+
EventSource source = manager.getResourceEventSourceFor(HasMetadata.class);
83+
assertThat(source).isInstanceOf(ControllerResourceEventSource.class);
84+
85+
assertThatExceptionOfType(IllegalArgumentException.class)
86+
.isThrownBy(() -> manager.getResourceEventSourceFor(HasMetadata.class, "unknown_name"));
8787

8888
CachingEventSource eventSource = mock(CachingEventSource.class);
8989
when(eventSource.resourceType()).thenReturn(String.class);
9090
manager.registerEventSource(eventSource);
9191

9292
source = manager.getResourceEventSourceFor(String.class);
93-
assertTrue(source.isPresent());
94-
assertEquals(eventSource, source.get());
93+
assertThat(source).isNotNull();
94+
assertEquals(eventSource, source);
9595
}
9696

9797
@Test
@@ -133,41 +133,12 @@ void retrievingAnEventSourceWhenMultipleAreRegisteredForATypeShouldRequireAQuali
133133
assertTrue(exception.getMessage().contains("name1"));
134134
assertTrue(exception.getMessage().contains("name2"));
135135

136-
assertEquals(manager.getResourceEventSourceFor(TestCustomResource.class, "name2").get(),
136+
assertEquals(manager.getResourceEventSourceFor(TestCustomResource.class, "name2"),
137137
eventSource2);
138-
assertEquals(manager.getResourceEventSourceFor(TestCustomResource.class, "name1").get(),
138+
assertEquals(manager.getResourceEventSourceFor(TestCustomResource.class, "name1"),
139139
eventSource);
140140
}
141141

142-
@Test
143-
void timerAndControllerEventSourcesShouldBeListedFirst() {
144-
EventSourceManager manager = initManager();
145-
146-
CachingEventSource eventSource = mock(CachingEventSource.class);
147-
when(eventSource.resourceType()).thenReturn(String.class);
148-
manager.registerEventSource(eventSource);
149-
150-
final Set<EventSource> sources = manager.getRegisteredEventSources();
151-
assertEquals(3, sources.size());
152-
final Iterator<EventSource> iterator = sources.iterator();
153-
for (int i = 0; i < sources.size(); i++) {
154-
final EventSource source = iterator.next();
155-
switch (i) {
156-
case 0:
157-
assertTrue(source instanceof TimerEventSource);
158-
break;
159-
case 1:
160-
assertTrue(source instanceof ControllerResourceEventSource);
161-
break;
162-
case 2:
163-
assertTrue(source instanceof CachingEventSource);
164-
break;
165-
default:
166-
fail();
167-
}
168-
}
169-
}
170-
171142
private EventSourceManager initManager() {
172143
final ControllerConfiguration configuration = mock(ControllerConfiguration.class);
173144
when(configuration.getResourceClass()).thenReturn(HasMetadata.class);

‎operator-framework-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourcesTest.java

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
package io.javaoperatorsdk.operator.processing.event;
2+
3+
import org.junit.jupiter.api.Test;
4+
5+
import io.javaoperatorsdk.operator.processing.event.source.EventSource;
6+
7+
import static org.junit.jupiter.api.Assertions.*;
8+
import static org.mockito.Mockito.mock;
9+
10+
class EventSourcesTest {
11+
12+
EventSources eventSources = new EventSources();
13+
14+
@Test
15+
void cannotAddTwoEventSourcesWithSameName() {
16+
assertThrows(IllegalArgumentException.class, () -> {
17+
eventSources.add("name", mock(EventSource.class));
18+
eventSources.add("name", mock(EventSource.class));
19+
});
20+
}
21+
22+
}

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/createupdateeventfilter/CreateUpdateEventFilterTestReconciler.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public Map<String, EventSource> prepareEventSources(
106106
.withLabelSelector("integrationtest = " + this.getClass().getSimpleName())
107107
.build();
108108
informerEventSource = new InformerEventSource<>(informerConfiguration, client);
109-
return EventSourceInitializer.defaultNamedEventSources(informerEventSource);
109+
return EventSourceInitializer.nameEventSources(informerEventSource);
110110
}
111111

112112
@Override

‎operator-framework/src/test/java/io/javaoperatorsdk/operator/sample/informereventsource/InformerEventSourceTestCustomReconciler.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ public Map<String, EventSource> prepareEventSources(
4747
.build();
4848

4949
return EventSourceInitializer
50-
.defaultNamedEventSources(new InformerEventSource<>(config, context));
50+
.nameEventSources(new InformerEventSource<>(config, context));
5151
}
5252

5353
@Override

0 commit comments

Comments
 (0)
0