Event Notification and Polling, Inbound Event Sources #720
Conversation
csviri
force-pushed
the
polling-event-source
branch
from
6e8b42a
to
c713574
Compare
csviri
force-pushed
the
polling-event-source
branch
2 times, most recently
from
37f46bb
to
d73917b
Compare
|
Refactored the MySQL example to use the |
This reverts commit 07f3053.
metacosm
force-pushed
the
polling-event-source
branch
from
c0cef5f
to
e5b32f6
Compare
|
|
||
| protected void handleDelete(ResourceID relatedResourceID) { | ||
| if (!isRunning()) { | ||
| log.debug("Received event but event for {} source is not running", relatedResourceID); |
There was a problem hiding this comment.
This logging doesn't make sense
There was a problem hiding this comment.
The though was to make is easier on debug level to find out the life cycle of an event. If you prefer I can remove this.
There was a problem hiding this comment.
I meant the message that's output… I'm not sure what it means :)
|
|
||
| protected void handleEvent(T value, ResourceID relatedResourceID) { | ||
| if (!isRunning()) { | ||
| log.debug("Received event but event for {} source is not running", relatedResourceID); |
There was a problem hiding this comment.
This logging doesn't make sense
| cache.remove(relatedResourceID); | ||
| // we only propagate event if the resource was previously in cache | ||
| if (cachedValue != null | ||
| && (eventFilter == null || eventFilter.acceptDelete(cachedValue, relatedResourceID))) { |
There was a problem hiding this comment.
Why would you not want to propagate a delete event?
There was a problem hiding this comment.
Good question. Wanted to make a generic filter, but this might be over engineered. We can remove the filters for now and see if someone is asking for it.
|
|
||
| import io.javaoperatorsdk.operator.processing.event.ResourceID; | ||
|
|
||
| public interface EventFilter<T> { |
There was a problem hiding this comment.
The concept of event filter actually doesn't really make much sense to me… the events come from event sources which you write, so presumably, the event source decide of which events trigger the event handler so I'm not sure why we need to filter them again…
There was a problem hiding this comment.
Same as above, the idea was to separate that two thing, maybe to reuse some filters. But agree, I will remove filtering
| * | ||
| * @param resourceID of the target related resource | ||
| * @return the cached value of the resource, if not present it gets the resource from the | ||
| * supplier. If the supplier provides a value it is cached, so there will be no new event |
There was a problem hiding this comment.
The last sentence is not very clear…
| values.forEach((k, v) -> super.handleEvent(v, k)); | ||
| var keysToRemove = StreamSupport.stream(cache.spliterator(), false) | ||
| .filter(e -> !values.containsKey(e.getKey())).map(Cache.Entry::getKey) | ||
| .collect(Collectors.toList()); |
There was a problem hiding this comment.
why not remove the keys directly instead of collecting them in a list first?
|
|
||
| public MySQLSchemaReconciler(KubernetesClient kubernetesClient, MySQLDbConfig mysqlDbConfig) { | ||
| this.kubernetesClient = kubernetesClient; | ||
| this.mysqlDbConfig = mysqlDbConfig; | ||
| } | ||
|
|
||
| @Override | ||
| public void prepareEventSources(EventSourceRegistry<MySQLSchema> eventSourceRegistry) { |
There was a problem hiding this comment.
It seems that this new implementation actually makes things more complicated than before: you have to write more code to achieve the same result (or at least, that's how it looks to me). Granted, I'm not familiar with this example / use case at all but it doesn't appear simpler now.
There was a problem hiding this comment.
But for now it was not receiving events if the schema was deleted or changed.
There was a problem hiding this comment.
OK so that's probably what I'm missing 😄
Like I said, I'm not really familiar with the example so just looking at the code seemed like things were now more complex.
There was a problem hiding this comment.
I also need to think about how to integrate this new feature with the dependent resources work.
No description provided.