improve: extend SecondaryToPrimaryMapper so it also gets the old resource

[csviri]

This might needed in some corner cases where might help with optimizations to which resource to trigger.

TODO adaptor

Signed-off-by: Attila Mészáros a_meszaros@apple.com

[csviri]

see also discussio here: https://discord.com/channels/723455000604573736/723455000604573739/1508830486913482913

[csviri]

as discussed with @dvob , he will created a detailed issue for this.

[dvob]

@csviri thank you for implementing this so quickly. I also created issue #3401 for reference which we can more or less close right away if this is merged.

I updated my reproducer to use two different types (MyThing and MyThingConfig), in the hope that it is more understandable. I also updated it to use this PR and it did work (test does no longer fail) https://github.com/dvob/java-operator-sdk-repo-example/tree/csviri-secondary-to-primary-improve

However, when updating my reproducer, I ran into a different challenge. In my initial reproducer I used only one CRD (Repo) which referenced itself. But now I have to two different CRDs and in withPrimaryToSecondaryMapper I basically want to access the MyThingConfig informer which I'm building which is not easily possible. For now I did some dirty hack. But you might have a better idea to achieve this: https://github.com/dvob/java-operator-sdk-repo-example/blob/4b9e01c3099ae580036aa81cbe1558c0d422f0b0/src/main/java/com/example/MyThingReconciler.java#L53 ?

One thing which also came to my mind is, that if you not specify withPrimaryToSecondaryMapper it will use the DefaultPrimaryToSecondaryIndex which most likely will behave wrong in such a scenario. This is not really a problem, as you just can set withPrimaryToSecondaryMapper, but I'm just no sure how obvious this is for users of the API.

[csviri]

Yes, not sure if we can do better than to document it,see:

https://javaoperatorsdk.io/docs/documentation/working-with-es-caches/#the-use-case-for-primarytosecondarymapper

https://javaoperatorsdk.io/docs/documentation/eventing/#managing-relation-between-primary-and-secondary-resources

[dvob]

Yes, I think docs are fine.

For a long time i didn't even knew about the default reverse index behavior. I did set the primaryToSecondaryMapper every time explicitly and I still do this because I think its easier to understand if things are a little more explicit. I don't know how other users rely on this.

[csviri]

I did set the primaryToSecondaryMapper every time explicitly and I still do this because I think its easier to understand if things are a little more explicit. I don't know how other users rely on this.

Yes, unfortunately this is not a that nice and round concept, but currently the only way to be able to use getSecondaryResource uniformly. We time to time get questions around it so it is used.

[csviri]

basically want to access the MyThingConfig informer which I'm building which is not easily possible. For now I did some dirty hack. But you might have a better idea to achieve this: https://github.com/dvob/java-operator-sdk-repo-example/blob/4b9e01c3099ae580036aa81cbe1558c0d422f0b0/src/main/java/com/example/MyThingReconciler.java#L53 ?

Please take a look on indexers, setting a custom indexer for that would solve the problem as far I can see. InformerEventSource has API to add indexer:

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

Lines 227 to 233 in 064799e

	@Override
	public void addIndexers(Map<String, Function<R, List<String>>> indexers) {
	if (isRunning()) {
	throw new OperatorException("Cannot add indexers after InformerEventSource started.");
	}
	this.indexers.putAll(indexers);
	}

then access the resource by index:

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

Line 276 in 064799e

public List<R> byIndex(String indexName, String indexKey) {

[csviri]

I will add an Integration test to showcase your sample handling

[csviri]

replaced by: #3425

[csviri]

@dvob I implemented an alternative version with an integration test to cover your use case, if everything goes well, this will be in next minor release. Please take a look, and let me know if it looks good to you.