fix: Improve Circuit breaker behaviour

[Jonas-Isr]

Context

SAP/cloud-sdk-java-backlog#495

This PR improves the circuit breaker logic by prepending an additional check to tryGetDestination. When a single destination is called, it is first checked whether this destination appears in the result of a get all destination call. If the destination is not present, no call to the destination is made. Thus the circuit breaker does not open when non-existing destination(s) are called multiple times.

This new behaviour is implemented as opt-in to not introduce breaking changes.

Feature scope:

• Add additional check
• add feature switch to turn on new additional check

Definition of Done

• Functionality scope stated & covered
• Tests cover the scope above
• Error handling created / updated & covered by the tests above
• Documentation updated
  • Java 5.25.0: Pre-Lookup check (circuit breaker) cloud-sdk#2327
• Release notes updated

[newtork]

(Major)

Please consider different API options, to reflect on existing available methods.

- DestinationService.Cache.setGetAllDocumentsPrepended(true);

+ DestinationService.Cache.enableGetAllDocumentsPrepended();
+ DestinationService.Cache.enableGetAllDestinationsPrepended();
+ DestinationService.Cache.enablePreLookupCheck();
+ DestinationService.Cache.enableExistenceCheck();

Also your feature would be enabled by default - right?

[Jonas-Isr]

Renamed it to DestinationService.Cache.enablePreLookupCheck().

Right now I have so that the behaviour is disabled by default. I did that to not introduce breaking changes (and also we talked about that this might increase response times for the first call). Do you think it should be enabled by default?

[newtork]

(Question/Minor)

Not a huge fan of Boolean (sic) prependGetAllDestinationCall additional 7th argument.
Have you considered wrapping this check into destinationRetriever argument before calling the method? If it's not too complex/too much additional code (likely).

[Jonas-Isr]

Boolean was a typo, should of course have been boolean 😬

I struggled a bit with placing this into destinationRetriever but found another solution by putting it directly into DestinationService.tryGetDestination. Is there anything that speaks against this?

[rpanackal]

(Question)

So the idea was instead of directly augmenting resilience config cache key, we introduce a (opt-in) check for the existence of a destination, essentially avoid tripping the circuit breaker in the first place.

But, I wonder what if a destination call fails for a different reason than DestinationAccessException? Is that possible? If so, I don't see how the current approach would resolve the issue?

[Jonas-Isr]

But, I wonder what if a destination call fails for a different reason than DestinationAccessException? Is that possible? If so, I don't see how the current approach would resolve the issue?

Yes, this is indeed possible and this approach does not solve the issue. We discussed this and came to the conclusion that the problem we solve with the current approach is the main problem we want to solve. Solving the issue you describe here might lead to unexpected behaviour of the SDK so we decided against trying to solve that. Our hope was that by adding this additional check we solve the main amount of problems.

I was made aware we want this to be "opt-out" instead of "opt-in" (?)

[Jonas-Isr]

I was made aware we want this to be "opt-out" instead of "opt-in" (?)

I adapted the code accordingly.

Also, I would say it is not necessary to write specific docs for this (besides the release notes). Do you agree?

I wrote some docs now.

[rpanackal]

LGTM