feat: add resource fetcher to CachingInboundEventSource

[scrocquesel]

Refs: #1424

Signature change but I doubt anyone use this event source as it is not currently working well.

Expiration needs a single fetcher by secondary, So I prefered seperating the behavior in a another event source as not everyone will need this.

[csviri]

Thank you!
The implementation looks good to me, could you add unit tests pls?

[csviri]

Maybe take a look on:
https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)

This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted. But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

[scrocquesel]

Maybe take a look on:

https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)

This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted.

That's already there.

But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

Indeed, I make another PR but it's more complicated than it appears. Doing a period fetching will become no more than a PerResourcePolling (with inbound event on top to maybe get fresh date quicker) and it's not really what this event source should do IMHO. It should only fetch data in the cache when required and not update the cache proactively when it become stale.

[scrocquesel]

Thank you! The implementation looks good to me, could you add unit tests pls?

I know you will ask for but as there wasn't yet, I secretly hope you will not :)
I think it also miss an integration test to show how to use it. Should the class be derived and start/stop overriden to start/stop a web listener and bind request to the public handle mthod. Or should it be share somehow between the operator and a web listener. I'm especially wondering how it could be integrated in a quarkus operator with quarkus rest services or websocket.

[csviri]

Or should it be share somehow between the operator and a web listener.

It should, I think operator should not deal at this point with servlets.

[csviri]

Maybe take a look on:
https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)
This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted.

That's already there.

Ahh nice.
Shoudn't we fetch the resource for onResourceCreated (async way, so the Thread is not blocked) ? So the reconciliation is faster.

But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

Indeed, I make another PR but it's more complicated than it appears. Doing a period fetching will become no more than a PerResourcePolling (with inbound event on top to maybe get fresh date quicker) and it's not really what this event source should do IMHO. It should only fetch data in the cache when required and not update the cache proactively when it become stale.

Ok, agree, let's stay with the simple approach for now.

[scrocquesel]

Maybe take a look on:
https://github.com/java-operator-sdk/java-operator-sdk/blob/7603d0374400ac804ac85c09c7b866851f2d5ad7/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ResourceEventAware.java#L5-L5

(Used in PerResourcePollingEventSource)
This might be an interesting aspect of it, so the cleanup is called if custom resource is deleted.

That's already there.

Ahh nice. Shoudn't we fetch the resource for onResourceCreated (async way, so the Thread is not blocked) ? So the reconciliation is faster.

It will be fetched by the getSecondaryResources ? What would be the race condition in this case ? Will it wait for the parrallel fetch to complete ?

But the first fetching (maybe event an optional periodic fetching in the background might be some nice addition. But probably in an other PR :)

Indeed, I make another PR but it's more complicated than it appears. Doing a period fetching will become no more than a PerResourcePolling (with inbound event on top to maybe get fresh date quicker) and it's not really what this event source should do IMHO. It should only fetch data in the cache when required and not update the cache proactively when it become stale.

Ok, agree, let's stay with the simple approach for now.

[scrocquesel]

@csviri Had to use the Set.of overload to avoid replacing the existing secondary. I guess the correct behavior should be additive.

source.handleResourceEvent(primaryID, a)
source.handleResourceEvent(primaryID, b)
source.handleResourceEvent(primaryID, c)
source.handleResourceDeleteEvent(primaryID, c)
source.getSecondaryResources(testCustomResource) // should return (a, b)
source.handleResourceEvent(primaryID, Set.of(x,y))
source.getSecondaryResources(testCustomResource) // should return (x,y)

Wait for your confirmation and will add javadoc so it's more clear to the user

[csviri]

No it should not be. At least that is not what I had on my mind.

[csviri]

Probably that would be good to document.
We could add overloaded or rather differerently named versions for additiveness, that would be useful for sure. But this is on purpose, now.

[csviri]

LGTM

[csviri]

@metacosm this LGTM, pls take a look if you will find some time.

[csviri]

@scrocquesel could you rebase this to the next branch? and target next with the PR. next contains the next minor release with new features, main is usually there for patching.