Skip to content

fix: Improve Circuit breaker behaviour #1017

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.

Sign up for GitHub

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

Merged
Jonas-Isr merged 23 commits into from
Dec 16, 2025
Merged

fix: Improve Circuit breaker behaviour #1017

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
b43e388
feat: Circuit Breaker thing
CharlesDuboisSAP Nov 26, 2025
87aab24
tests green
Jonas-Isr Nov 26, 2025
0ec0870
add feature switch
Jonas-Isr Nov 27, 2025
7b0e572
add tests for feature
Jonas-Isr Nov 27, 2025
077b784
remove other changes from tests
Jonas-Isr Nov 27, 2025
75e300b
code style
Jonas-Isr Nov 27, 2025
5475777
PMD
Jonas-Isr Nov 27, 2025
2501904
move feature switch
Jonas-Isr Nov 28, 2025
31924b3
codestyle
Jonas-Isr Nov 28, 2025
4ab435e
codestyle
Jonas-Isr Dec 1, 2025
a3b144f
formatting
Jonas-Isr Dec 2, 2025
689acb8
reviews
Jonas-Isr Dec 2, 2025
624c10a
javadoc
Jonas-Isr Dec 2, 2025
3e05498
Merge branch 'main' into circuit-breaker
Jonas-Isr Dec 3, 2025
d573e51
codestyle
Jonas-Isr Dec 3, 2025
6cf6d0f
release notes
Jonas-Isr Dec 3, 2025
d8f9139
PMD
Jonas-Isr Dec 3, 2025
d62b970
reviews + enable by default
Jonas-Isr Dec 8, 2025
5ba5226
reviews
Jonas-Isr Dec 8, 2025
0bf62f6
reviews
Jonas-Isr Dec 9, 2025
7b6e46e
reviews
Jonas-Isr Dec 10, 2025
1bc4f01
Merge branch 'main' into circuit-breaker
Jonas-Isr Dec 10, 2025
2aef26b
codestyle
Jonas-Isr Dec 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 34 additions & 8 deletions ...ervice/src/main/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.RequiredArgsConstructor;
import lombok.Setter;
import lombok.extern.slf4j.Slf4j;
import lombok.val;

/**
* Retrieves destination information from the SCP destination service on Cloud Foundry.
Expand Down Expand Up @@ -121,6 +121,10 @@ static ResilienceConfiguration createResilienceConfiguration(
Try<Destination>
tryGetDestination( @Nonnull final String destinationName, @Nonnull final DestinationOptions options )
{
if( Cache.preLookupCheckEnabled && !preLookupCheckSuccessful(destinationName) ) {
final String msg = "Destination %s was not found among the destinations of the current tenant.";
return Try.failure(new DestinationAccessException(String.format(msg, destinationName)));
Comment thread
Jonas-Isr marked this conversation as resolved.
Outdated
}
return Cache.getOrComputeDestination(this, destinationName, options, this::loadAndParseDestination);
}

Expand Down Expand Up @@ -385,6 +389,17 @@ private static boolean hasCauseAssignableFrom( @Nonnull final Throwable t, @Nonn
return ExceptionUtils.getThrowableList(t).stream().map(Throwable::getClass).anyMatch(cls::isAssignableFrom);
}

private boolean preLookupCheckSuccessful( final String destinationName )
Comment thread
Jonas-Isr marked this conversation as resolved.
{
val allDestinations = getAllDestinationProperties();
if( !allDestinations.isEmpty() ) {
return allDestinations
.stream()
.anyMatch(properties -> properties.get(DestinationProperty.NAME).contains(destinationName));
}
return false;
Comment thread
Jonas-Isr marked this conversation as resolved.
Outdated
}

/**
* Helper class that encapsulates all caching related configuration options.
*
Expand Down Expand Up @@ -427,8 +442,7 @@ public static final class Cache

private static boolean cacheEnabled = true;
private static boolean changeDetectionEnabled = true;
@Setter
private static boolean getAllDocumentsPrepended = false;
private static boolean preLookupCheckEnabled = false;

static {
recreateSingleCache();
Expand All @@ -446,9 +460,22 @@ static boolean isChangeDetectionEnabled()
return changeDetectionEnabled;
}

static boolean isGetAllDocumentsPrepended()
/**
* Enables checking if a destination exists before trying to call it directly when invoking
* {@link #tryGetDestination}.
Comment thread
Jonas-Isr marked this conversation as resolved.
Outdated
*/
public static void enablePreLookupCheck()
Comment thread
Jonas-Isr marked this conversation as resolved.
Outdated
{
preLookupCheckEnabled = true;
}

/**
* Disables checking if a destination exists before trying to call it directly when invoking
* {@link #tryGetDestination}.
*/
public static void disablePreLookupCheck()
{
return getAllDocumentsPrepended;
preLookupCheckEnabled = false;
}

@Nonnull
Expand Down Expand Up @@ -504,7 +531,7 @@ static void reset()
cacheEnabled = true;
}
changeDetectionEnabled = true;
getAllDocumentsPrepended = false;
preLookupCheckEnabled = false;

sizeLimit = Option.some(DEFAULT_SIZE_LIMIT);
expirationDuration = Option.some(DEFAULT_EXPIRATION_DURATION);
Expand Down Expand Up @@ -875,8 +902,7 @@ private static Try<Destination> getOrComputeDestination(
instanceSingle(),
isolationLocks(),
destinationDownloader,
getAllCommand,
getAllDocumentsPrepended);
getAllCommand);
return command.flatMap(GetOrComputeSingleDestinationCommand::execute);
}

Expand Down
27 changes: 2 additions & 25 deletions ...va/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeSingleDestinationCommand.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
import lombok.Getter;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import lombok.val;

@Slf4j
@RequiredArgsConstructor( access = AccessLevel.PRIVATE )
Expand All @@ -53,8 +52,6 @@ class GetOrComputeSingleDestinationCommand
private final DestinationServiceTokenExchangeStrategy exchangeStrategy;
@Nullable
private final GetOrComputeAllDestinationsCommand getAllCommand;
@Nonnull
private final Boolean prependGetAllDestinationCall;

@SuppressWarnings( "deprecation" )
static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
Expand All @@ -63,8 +60,7 @@ static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
@Nonnull final Cache<CacheKey, Destination> destinationCache,
@Nonnull final Cache<CacheKey, ReentrantLock> isolationLocks,
@Nonnull final BiFunction<String, DestinationOptions, Destination> destinationRetriever,
@Nullable final GetOrComputeAllDestinationsCommand getAllCommand,
@Nonnull final Boolean prependGetAllDestinationCall )
@Nullable final GetOrComputeAllDestinationsCommand getAllCommand )
{
final Supplier<Destination> destinationSupplier =
() -> destinationRetriever.apply(destinationName, destinationOptions);
Expand Down Expand Up @@ -113,8 +109,7 @@ static Try<GetOrComputeSingleDestinationCommand> prepareCommand(
destinationCache,
destinationSupplier,
exchangeStrategy,
getAllCommand,
prependGetAllDestinationCall));
getAllCommand));
}

/**
Expand Down Expand Up @@ -154,10 +149,6 @@ Try<Destination> execute()
if( result != null ) {
return Try.success(result);
}
if( prependGetAllDestinationCall && !destinationExists() ) {
final String msg = "Destination %s was not found among the destinations of the current tenant.";
return Try.failure(new DestinationAccessException(String.format(msg, destinationName)));
}

final Try<Destination> maybeResult = Try.ofSupplier(destinationSupplier);
if( maybeResult.isFailure() ) {
Expand Down Expand Up @@ -272,20 +263,6 @@ private Destination getCachedDestination()
return maybeDestination;
}

private boolean destinationExists()
{
if( getAllCommand != null ) {
val allDestinations = getAllCommand.execute();
if( allDestinations != null && allDestinations.isSuccess() ) {
return allDestinations
.get()
.stream()
.anyMatch(properties -> properties.get(DestinationProperty.NAME).contains(destinationName));
}
}
return false;
}

/**
* currently this effectively limits the cache duration to the change detection interval, because change detection
* on certificates isn't possible in all cases See the note on {@link DestinationServiceFactory} where this property
Expand Down
8 changes: 4 additions & 4 deletions ...ce/src/test/java/com/sap/cloud/sdk/cloudplatform/connectivity/DestinationServiceTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1930,7 +1930,7 @@ void testPrependGetAllDestinationsCall()
.when(destinationServiceAdapter)
.getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());

DestinationService.Cache.setGetAllDocumentsPrepended(true);
DestinationService.Cache.enablePreLookupCheck();

final DestinationOptions options =
DestinationOptions.builder().augmentBuilder(augmenter().retrievalStrategy(ALWAYS_PROVIDER)).build();
Expand All @@ -1943,7 +1943,7 @@ void testPrependGetAllDestinationsCall()
.getConfigurationAsJson(eq("/v1/destinations/" + destinationName), any());
verifyNoMoreInteractions(destinationServiceAdapter);

DestinationService.Cache.setGetAllDocumentsPrepended(false);
DestinationService.Cache.disablePreLookupCheck();
}

@Test
Expand All @@ -1956,7 +1956,7 @@ void testPrependGetAllDestinationsCallWithMissingDestination()
.when(destinationServiceAdapter)
.getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());

DestinationService.Cache.setGetAllDocumentsPrepended(true);
DestinationService.Cache.enablePreLookupCheck();

final DestinationOptions options =
DestinationOptions.builder().augmentBuilder(augmenter().retrievalStrategy(ALWAYS_PROVIDER)).build();
Expand All @@ -1969,6 +1969,6 @@ void testPrependGetAllDestinationsCallWithMissingDestination()
verify(destinationServiceAdapter, times(1)).getConfigurationAsJson(eq("/v1/subaccountDestinations"), any());
verifyNoMoreInteractions(destinationServiceAdapter);

DestinationService.Cache.setGetAllDocumentsPrepended(false);
DestinationService.Cache.disablePreLookupCheck();
}
}
42 changes: 11 additions & 31 deletions ...java/com/sap/cloud/sdk/cloudplatform/connectivity/GetOrComputeDestinationCommandTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,14 +72,7 @@ void testDefaultTokenExchangeStrategy()

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand(
"destination",
options,
destinationCache,
isolationLocks,
( foo, bar ) -> null,
null,
false)
.prepareCommand("destination", options, destinationCache, isolationLocks, ( foo, bar ) -> null, null)
.get();

assertThat(sut.getExchangeStrategy()).isEqualTo(DestinationServiceTokenExchangeStrategy.LOOKUP_THEN_EXCHANGE);
Expand Down Expand Up @@ -165,7 +158,7 @@ private void assertCacheKeysMatchExchangeStrategy(

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.get();

assertThat(sut.getCacheKey()).isEqualTo(expectedCacheKey);
Expand Down Expand Up @@ -205,7 +198,7 @@ void testLookupOnlyOnUserTokenDestination()

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.get();

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -243,7 +236,7 @@ void testLookupOnlyExchangeStrategy()

final GetOrComputeSingleDestinationCommand sut =
GetOrComputeSingleDestinationCommand
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null, false)
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.get();

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -295,8 +288,7 @@ void testLookupThenExchangeStrategy()
destinationCache,
isolationLocks,
function,
null,
false)
null)
.get()));

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -350,8 +342,7 @@ void testForwardUserTokenStrategyForUserPropagationDestination()
destinationCache,
isolationLocks,
function,
null,
false)
null)
.get()));

final Try<Destination> fetchedDestination =
Expand Down Expand Up @@ -402,8 +393,7 @@ void testForwardUserTokenStrategyForUserPropagationDestinationWithoutJwt()
destinationCache,
isolationLocks,
function,
null,
false)
null)
.get()));

final Try<Destination> fetchedDestination = sut.execute();
Expand Down Expand Up @@ -447,14 +437,7 @@ void testForwardUserTokenStrategyForUserPropagationDestinationWithoutPrincipalUn
.executeWithTenant(
t1,
() -> GetOrComputeSingleDestinationCommand
.prepareCommand(
DESTINATION_NAME,
options,
destinationCache,
isolationLocks,
function,
null,
false)
.prepareCommand(DESTINATION_NAME, options, destinationCache, isolationLocks, function, null)
.get());

final Try<Destination> shouldBeFailure = TenantAccessor.executeWithTenant(t1, sut::execute);
Expand Down Expand Up @@ -504,8 +487,7 @@ void testForwardUserTokenStrategyForClientCredentialsDestination()
destinationCache,
isolationLocks,
function,
null,
false)
null)
.get()));

final Try<Destination> fetchedDestination = PrincipalAccessor.executeWithPrincipal(p1, () -> sut.execute());
Expand Down Expand Up @@ -560,8 +542,7 @@ void testChangeDetectionOnValidAuthToken()
destinationCache,
isolationLocks,
destinationRetriever,
getAllCommand,
false)
getAllCommand)
.flatMap(GetOrComputeSingleDestinationCommand::execute);

assertThat(result.isSuccess()).isTrue();
Expand Down Expand Up @@ -646,8 +627,7 @@ void testFirstDestinationAccessDoesNotTriggerGetAllRequest()
destinationCache,
isolationLocks,
destinationRetriever,
getAllMock,
false)
getAllMock)
.get();
final Try<Destination> result = sut.execute();

Expand Down
3 changes: 1 addition & 2 deletions ...platform/connectivity/GetOrComputeSingleDestinationCommandWithoutAllDestinationsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,8 +233,7 @@ private void runTest( TestCase testCase )
destinationCache,
isolationLocks,
destinationService::loadAndParseDestination,
null,
false);
null);

if( maybeCommand.isFailure() ) {
if( testCase.isCommandCreationExpectedToSucceed() ) {
Expand Down
2 changes: 1 addition & 1 deletion release_notes.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@

### ✨ New Functionality

-
- Added new functionality to check if a destination exists before trying to call it directly when invoking `DestinationService.tryGetDestination`.
Comment thread
Jonas-Isr marked this conversation as resolved.
Outdated

### 📈 Improvements

Expand Down