-
Notifications
You must be signed in to change notification settings - Fork 220
Refined Interface of EventSource
and EventSourceManager
#597
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
Changes from 44 commits
898e2c2
be4e5cf
b2ab4b9
9145b52
4ef27bf
34cc2a1
4b15974
77033e6
e1b5926
e7d1b99
a1f92c6
e928a3e
f35a340
5d5817e
544ce35
75ad7d2
7012fb3
3746122
f0f2e91
6638a48
1d786ef
a5343b7
9e0430a
7f48b9a
0aa29a1
3ad2fc5
86b8185
92d3ed3
d194d25
2e8f219
9ebfff0
f1f3867
b99ff10
326f82f
98e3def
a03cfb9
746f0c1
bb6c00e
80e8238
bbecf4c
26b1c3e
f350f46
d3e2469
974c0eb
b8478e2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,8 +2,7 @@ | |
|
||
import java.io.Closeable; | ||
import java.io.IOException; | ||
import java.util.Map; | ||
import java.util.Optional; | ||
import java.util.List; | ||
|
||
import io.fabric8.kubernetes.client.CustomResource; | ||
import io.javaoperatorsdk.operator.OperatorException; | ||
|
@@ -14,29 +13,15 @@ public interface EventSourceManager<T extends CustomResource<?, ?>> extends Clos | |
/** | ||
* Add the {@link EventSource} identified by the given <code>name</code> to the event manager. | ||
* | ||
* @param name the name of the {@link EventSource} to add | ||
* @param eventSource the {@link EventSource} to register | ||
* @throws IllegalStateException if an {@link EventSource} with the same name is already | ||
* registered. | ||
* @throws OperatorException if an error occurred during the registration process | ||
*/ | ||
void registerEventSource(String name, EventSource eventSource) | ||
void registerEventSource(EventSource eventSource) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we also make it possible to remove an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it should not. So were thinking how to handle some dynamic registration and de-registration of event sources. But that would make sense only per custom resource. Since we want to cover events for all related custom resources which are in differenet lifecycle state. So deregistering an event source will mean that new custom resources would not receive events about related dependent resource anymore, even the custom resource was just created. I see it rather this way: notifications for event sources about custom resource lifecycle, what we already partially cover:
|
||
throws IllegalStateException, OperatorException; | ||
|
||
/** | ||
* Remove the {@link EventSource} identified by the given <code>name</code> from the event | ||
* manager. | ||
* | ||
* @param name the name of the {@link EventSource} to remove | ||
* @return an optional {@link EventSource} which would be empty if no {@link EventSource} have | ||
* been registered with the given name. | ||
*/ | ||
Optional<EventSource> deRegisterEventSource(String name); | ||
|
||
Optional<EventSource> deRegisterCustomResourceFromEventSource( | ||
String name, CustomResourceID customResourceUid); | ||
|
||
Map<String, EventSource> getRegisteredEventSources(); | ||
List<EventSource> getRegisteredEventSources(); | ||
|
||
CustomResourceEventSource<T> getCustomResourceEventSource(); | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be a
Set
, actually? We don't need the ordering but we'd rather have only one instead of theEventSource
in the collection…There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, will change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed.