feat: external id provider for external dependent resources

[csviri]

This is the rough idea how to make the selection of target external dependent resources more visible.

[csviri]

cc @florianknecht @metacosm @xstefank

[florianknecht]

Continuing the ongoing discussion from #2968:

The old approach might miss the existing dependent resource if it drifts from desired externally. I hope this snippet of pseudocode can make my hypothesis clear.

cache = { myRes = {a, b} }
desired = { myRes = {a, b} }

// somebody changes myRes manually to be "{a, c}". Informer/Polling/Whatever triggers:
fetchResources() // results in writing cache = { myRes = {a, c} }

// a little later but before update() has been successfully called ...
currentResource = context.getSecondaryResource(primary) // empty, because desired !equals cache

This means we currently cannot get a resource from the cache if it is out of sync with the primary. This might be desirable as we might not want to rely on the caches data in this case - or it might not be because we could concurrently trigger a "create()" in some weird circumstance, thinking the resource is gone.

You're new approach might then be preferable - only requiring the id to match. Could we be repurposing/moving the "keyFor(R)" method from CacheKeyMapper for this?

[csviri]

not sure if following this part:

// a little later but before update() has been successfully called ...
currentResource = context.getSecondaryResource(primary) // empty, because desired !equals cache

This (context.getSecondaryResources(..)) should always return ALL the related resources to the primary, since that happens based on the ResourceID of the primary,see:

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/event/source/ExternalResourceCachingEventSource.java

Line 50 in 75405bf

protected Map<ResourceID, Map<String, R>> cache = new ConcurrentHashMap<>();

[florianknecht]

I hope I can make it more clear.
In my Example Primary ResourceID = "ID1" (oversimplifying here), Primary desired = "A".
Actual Resource has been changed out of band to "B" and needs to be reconciled. Cache has already been updated by e.g. Polling EventSource.

The cache now contains "ID1": "B". desired(primary) still is "A"

context.getSecondaryResource(...) (!) will end up in getTargetSecondaryResource which would effectively evaluate
context.getSecondaryResource**s**().stream().filter( desired.equals(resource) ) - which is empty.

The IDs match, and the cache has the resource, but the old implementation using equals() filters the object if cache(primary) != desired(primary).

[csviri]

I hope I can make it more clear. In my Example Primary ResourceID = "ID1" (oversimplifying here), Primary desired = "A". Actual Resource has been changed out of band to "B" and needs to be reconciled. Cache has already been updated by e.g. Polling EventSource.

The cache now contains "ID1": "B". desired(primary) still is "A"

context.getSecondaryResource(...) (!) will end up in getTargetSecondaryResource which would effectively evaluate context.getSecondaryResource**s**().stream().filter( desired.equals(resource) ) - which is empty.

The IDs match, and the cache has the resource, but the old implementation using equals() filters the object if cache(primary) != desired(primary).

This would not work even if you override equals on those objects? So it maches the ID that otherwise would be returned from the ExternalDependentIDProvider ?

[florianknecht]

Would overriding equals like this not prevent updates from working?

AbstractExternalResource.match() uses it to determine whether it needs to call update() on the resource, iirc.
If I only implemented an ID check in equals I suspect my code would never reconcile based on other fields being different on the remote.

[florianknecht]

As I said, I believe we could use the CacheKeyMapper interface instead of introducing a new one. We might want to rename it though.

[csviri]

Would overriding equals like this not prevent updates from working?

AbstractExternalResource.match() uses it to determine whether it needs to call update() on the resource, iirc. If I only implemented an ID check in equals I suspect my code would never reconcile based on other fields being different on the remote.

That is not true, this is just to selecting the target resource from cache, later there is a separate matching phase.

[csviri]

As I said, I believe we could use the CacheKeyMapper interface instead of introducing a new one. We might want to rename it though.

That is tempting, let me get back to this a bit later.

[florianknecht]

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java

Line 91 in 75405bf

return Matcher.Result.computed(resource.equals(desired), desired);

Is this the matching phase you mentioned? I was referring to this.
This would be the same resource.equals used in the getTargetSecondaryResource implementation, no? I might be mistaken on the code flow though.

[csviri]

java-operator-sdk/operator-framework-core/src/main/java/io/javaoperatorsdk/operator/processing/dependent/AbstractExternalDependentResource.java

Line 91 in 75405bf

return Matcher.Result.computed(resource.equals(desired), desired);

Is this the matching phase you mentioned? I was referring to this. This would be the same resource.equals used in the getTargetSecondaryResource implementation, no? I might be mistaken on the code flow though.

Hmm yes, that is true, that is meant to be overridden, we might want to have ti abstract in the future. But true, the default implementation is confusing this way. Yes, so you have to override that part. Will take a look how could we make this nicer in the defatult setup.

[florianknecht]

Cool, thanks a lot for the collaboration.

Just general feedback from me as a new user: I currently find it hard to determine what is meant to be overridden. I want to get the most out of the framework so I tend to leave defaults intact.
As a rule of thumb I'd prefer (interfaces + samples) over (abstract classes + default implementations) if the sdk expects me to override something.

Your input helps a ton, though. I've had a lot of progress with just these few back-and-forth messages. Thanks again for the support!

[csviri]

Cool, thanks a lot for the collaboration.

Just general feedback from me as a new user: I currently find it hard to determine what is meant to be overridden. I want to get the most out of the framework so I tend to leave defaults intact. As a rule of thumb I'd prefer (interfaces + samples) over (abstract classes + default implementations) if the sdk expects me to override something.

Your input helps a ton, though. I've had a lot of progress with just these few back-and-forth messages. Thanks again for the support!

Thank you!

yes, we will improve this for 5.2 soon definitely, so it is obvious how to handle this for users. Will create a separate an umbrella issue that has a summary of the problem a proposal how to handle it. thx again for the feedback and pointing this out!!

[csviri]

@florianknecht pls see also related issue: #2972

[csviri]

@florianknecht @metacosm @xstefank this is now ready for review. We might want to iterate more (separate PRs) on the cache keys and external resource states etc.

[xstefank]

Suggested change

	+ " selectTargetSecondaryResource.");
	+ " AbstractDependentResource#selectTargetSecondaryResource.");

[csviri]

since this is thrown from the method selectTargetSecondaryResource this might be more confusing than now

[xstefank]

I would appreciate to point where to find what I need to override but I'm ok with leaving it as is.

[csviri]

I updated the message to articulate that it is about "this", the method from which the exception is thrown.

[csviri]

Would be nice to update the MySQL sample to manage updates, can do it later, or as a separate PR.

[csviri]

@metacosm can I merge this?