Skip to content

Should be every dependent resource use/provide an Event Source and Cache the Actual State #1150

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

Closed
csviri opened this issue Apr 7, 2022 · 2 comments · Fixed by #1479
Closed
Assignees
Labels
architecture dependent-resources-epic needs-discussion Issue needs to be discussed more before working on it
Milestone

Comments

@csviri
Copy link
Collaborator

csviri commented Apr 7, 2022

Basically since DependentResource now has getResource() method, what by definition should return the actual state after reconciliation (so reconcile() method is called ) . Currently the resource is cached now usually in an event source, that most of the cases also serves as a cache.

The question is if it make sense to force this pattern in general, thus to have always an even source or even a caching event source for every dependent resource? See ResourceEventSource .

For Kubernetes resources we already have this cache in Informer, for external resources we have the CachingInboundEventSource for a resource oriented event coming from an external system. And two flavors of polling event source: PollingEventSource and PerResourcePollingEventSource.

What we don't have covered is the situation for an external resource when it is capable to notify about change, but the notification not contains the resource it self. Typically such external events are comming from AWS resources, like from RDS:
see: https://docs.aws.amazon.com/AmazonRDS/latest/UserGuide/rds-cloudwatch-events.sample.html
A sample event does not contains the resource itself.
( see also: Event Notification https://martinfowler.com/articles/201701-event-driven.html )

We have two options here either the dependent resource fetches the resource on reconcile and puts it into local cache. Or we prepare an even source that when receives an inbound event also fetches and caches the resource (then triggers and event).
This would be much more optimal, since we just don't fetch resource on every reconcile. So the event source logic should look like this:

sequenceDiagram
    participant InboundFetchingEventSource
    participant ExternalService
    participant EventHandler
    ExternalService->>InboundFetchingEventSource: Send Resource Changed Event
    InboundFetchingEventSource->>ExternalService: Fetch The Whole Resource
    InboundFetchingEventSource->>InboundFetchingEventSource: Cache The Resource
    InboundFetchingEventSource->>EventHandler: Propagate Event About The Change
Loading

Notes

see also: #1134
(note that this issue does not have that much impact as it seems, since even if there is separate caching layer, the event sources are propagating the resources to the cache.)

@csviri csviri added needs-discussion Issue needs to be discussed more before working on it architecture labels Apr 7, 2022
@csviri csviri self-assigned this Apr 7, 2022
@metacosm metacosm added this to the 3.1 milestone May 31, 2022
@csviri csviri modified the milestones: 3.1, 3.2 Jun 2, 2022
@csviri
Copy link
Collaborator Author

csviri commented Jun 21, 2022

Was thinking about this, reading the actual state is already from the context:
https://github.com/java-operator-sdk/java-operator-sdk/blob/464ca0262a9a296b3ea68fd1c59e18145ea1a296/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractDependentResource.java#L32-L32

It should be possible also to share event sources between dependent resources, in that case it might be better to create the event source separately, and turn off event source creation for all related resources.

With this issue we will anyways decouple event source from the secondary object access in the API:
#1240

@csviri csviri modified the milestones: 3.2, 3.3 Jun 28, 2022
@csviri csviri linked a pull request Sep 28, 2022 that will close this issue
@csviri csviri closed this as completed Oct 4, 2022
@csviri
Copy link
Collaborator Author

csviri commented Oct 4, 2022

Extracted the event source definition to this issue: #1514

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
architecture dependent-resources-epic needs-discussion Issue needs to be discussed more before working on it
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants