fix: event sources no ordered start & missing event source throws exception #1104
Conversation
csviri
commented
Mar 30, 2022
csviri
commented
- Event sources not need to start in particular order this is handled by the processor - it starts after event sources started
- If asked for a secondary resource from the context for a event source using the name, throws exception if event source not present with that name
|
see related PR where it caused issues: #1103 (issues were not recoginized) |
Are you sure? This used to be a problem…
👍🏼 |
…eption - Event sources not need to start in particular order this is handled by the processor - it starts after event sources started - If asked for a secondary resource from the context for a event source using the name, throws exception if event source not present with that name
csviri
force-pushed
the
simplify-secondary-from-context
branch
from
956af6b to
5498355
Compare
Yep, the fix in event processor happened after, so now the event processor start after each event source started. So no event processed before all of the started. (The issue before was that the ControllerEventSource did not start before than others, so the cache was not populated soon enough) |
...-core/src/test/java/io/javaoperatorsdk/operator/processing/event/EventSourceManagerTest.java
Outdated
Show resolved
Hide resolved
…tor/processing/event/EventSourceManagerTest.java Co-authored-by: Chris Laprun <metacosm@users.noreply.github.com>
| static String defaultNameFor(EventSource source) { | ||
| return source.getClass().getCanonicalName(); | ||
| // we can have multiple event sources for the same class | ||
| return source.getClass().getName() + "#" + source.hashCode(); |
There was a problem hiding this comment.
🤔 I'm not sure how this would work… Why do we need a name if the generated names are unique already? Also, this only works if the user already has access to the EventSource instance, which kind of defeats the purpose of being able to retrieve it by its name… so something doesn't really work here.
There was a problem hiding this comment.
The previous implementation simply does not work when there are multiple instances with the same type. Those would get the same name (there was already a bug with that) so basically not addressable. The default name is only for the case when the user does not want to explicitly address the event source.
So either:
- there is just 1 event source by type, so the resource is accessed via the context and type
- the cached resource addressed via a dependent resource
On every other cases it's just needs an explicit name, so the generated name is only basically a workaround to user does not need to think about a name is it's not used anywhere. It's correct in this sense.
There was a problem hiding this comment.
Note that InformerEventSoure for a config map and a deployment for example still has the same class.
There was a problem hiding this comment.
What could ne done is just move this to EventSourceInitializer method and make it more explicit that is generated. Would make ti a separate PR.
There was a problem hiding this comment.
ok pushed a small refactor regarding to this
…rator-sdk/java-operator-sdk into simplify-secondary-from-context
|
Kudos, SonarCloud Quality Gate passed! |
