Skip to content
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import com.azure.cosmos.implementation.LeaseNotFoundException;
import com.azure.cosmos.implementation.OperationType;
import com.azure.cosmos.implementation.PartitionIsMigratingException;
import com.azure.cosmos.implementation.PartitionKeyRangeGoneException;
import com.azure.cosmos.implementation.PartitionKeyRangeIsSplittingException;
import com.azure.cosmos.implementation.RequestTimeoutException;
import com.azure.cosmos.implementation.ResourceType;
Expand Down Expand Up @@ -325,6 +326,52 @@ public void shouldRetryWithPartitionKeyRangeIsSplittingException() {

}

/**
* Retry with PartitionKeyRangeGoneException
*/
@Test(groups = { "unit" }, timeOut = TIMEOUT)
public void shouldRetryWithPartitionKeyRangeGoneException() {
RxDocumentServiceRequest request = RxDocumentServiceRequest.create(
mockDiagnosticsClientContext(),
OperationType.Read,
ResourceType.Document);
GoneAndRetryWithRetryPolicy goneAndRetryWithRetryPolicy = new GoneAndRetryWithRetryPolicy(request, 30);
Mono<ShouldRetryResult> singleShouldRetry = goneAndRetryWithRetryPolicy
.shouldRetry(new PartitionKeyRangeGoneException());
ShouldRetryResult shouldRetryResult = singleShouldRetry.block();
assertThat(shouldRetryResult.shouldRetry).isTrue();
assertThat(request.forcePartitionKeyRangeRefresh).isTrue();
assertThat(request.requestContext.resolvedPartitionKeyRange).isNull();
assertThat(request.requestContext.quorumSelectedLSN).isEqualTo(-1);
assertThat(shouldRetryResult.policyArg.getValue0()).isFalse();
}

@Test(groups = { "unit" }, timeOut = TIMEOUT)
public void shouldWrapPartitionKeyRangeGoneExceptionWithServiceUnavailableWhenRetryBudgetExhausted() {
RxDocumentServiceRequest request = RxDocumentServiceRequest.create(
mockDiagnosticsClientContext(),
OperationType.Read,
ResourceType.Document);
GoneAndRetryWithRetryPolicy goneAndRetryWithRetryPolicy = new GoneAndRetryWithRetryPolicy(request, 0);

ShouldRetryResult shouldRetryResult = goneAndRetryWithRetryPolicy
.shouldRetry(new PartitionKeyRangeGoneException())
.block();
assertThat(shouldRetryResult.shouldRetry).isTrue();

shouldRetryResult = goneAndRetryWithRetryPolicy
.shouldRetry(new PartitionKeyRangeGoneException())
.block();

assertThat(shouldRetryResult.shouldRetry).isFalse();
assertThat(shouldRetryResult.exception).isInstanceOf(CosmosException.class);

CosmosException cosmosException = (CosmosException) shouldRetryResult.exception;
assertThat(cosmosException.getStatusCode()).isEqualTo(HttpConstants.StatusCodes.SERVICE_UNAVAILABLE);
assertThat(cosmosException.getSubStatusCode())
.isEqualTo(HttpConstants.SubStatusCodes.PARTITION_KEY_RANGE_GONE_EXCEEDED_RETRY_LIMIT);
}

/**
* No retry on bad request exception
*/
Expand Down
1 change: 1 addition & 0 deletions sdk/cosmos/azure-cosmos/CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
#### Breaking Changes

#### Bugs Fixed
* Fixed direct connectivity retries for `PartitionKeyRangeGoneException` thrown during address resolution so stale partition key ranges trigger routing-map refresh and exhausted retries surface as 503 with partition-key-range-gone retry-limit substatus. - See [Issue 49381](https://github.com/Azure/azure-sdk-for-java/issues/49381).

#### Other Changes
* Reduced memory footprint of deserialized `PartitionKeyRange` instances by stripping unused fields in the `PartitionKeyRange(ObjectNode)` constructor - See PR [49513](https://github.com/Azure/azure-sdk-for-java/pull/49513).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ private boolean isNonRetryableException(Exception exception) {
if (exception instanceof GoneException ||
exception instanceof PartitionIsMigratingException ||
exception instanceof PartitionKeyRangeIsSplittingException ||
exception instanceof PartitionKeyRangeGoneException ||
exception instanceof LeaseNotFoundException) {

return false;
Expand Down Expand Up @@ -292,6 +293,8 @@ private Pair<Mono<ShouldRetryResult>, Boolean> handleException(Exception excepti
return handlePartitionIsMigratingException((PartitionIsMigratingException)exception);
} else if (exception instanceof PartitionKeyRangeIsSplittingException) {
return handlePartitionKeyIsSplittingException((PartitionKeyRangeIsSplittingException) exception);
} else if (exception instanceof PartitionKeyRangeGoneException) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change would need adjoining FaultInjection tests as well to simulate e2e behavior - PKRangeGone as far as I recall is only happening for address lookups against Gateway - and is handled there. So, I don't think adding it to GoneAndRetryWithRetryPolicy is needed / helps.

@xinlian12 - can you double-check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @FabianMeiswinkel that makes sense. The intent here was to cover the case where direct-mode address resolution surfaces 410/1002 as PartitionKeyRangeGoneException before the request can be re-routed, so this policy clears the resolved PKRange and forces a PKRange refresh similar to split handling.

I agree the PR should prove that path with FaultInjection instead of only unit tests. I see FaultInjectionServerErrorType.PARTITION_IS_GONE maps to 410/1002, so I can add direct-mode FI coverage around that. If this exception is guaranteed to be handled before reaching GoneAndRetryWithRetryPolicy, then I’m happy to close/rework this PR instead. @xinlian12 could you confirm the expected layer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FabianMeiswinkel I tightened this so PartitionKeyRangeGoneException is not retried broadly anymore.

The retry path is now opt-in only for address-resolution-created exceptions:

  • AddressResolver marks the exception before throwing it
  • GatewayAddressCache does the same for the address-feed no-entry path
  • unmarked PartitionKeyRangeGoneException remains non-retryable, preserving the change feed / query behavior

I also added/updated unit coverage in GoneAndRetryWithRetryPolicyTest for marked vs unmarked PartitionKeyRangeGoneException.

I have not added a full FI/e2e test yet. Do you still want FI coverage for this path, or is the scoped marker approach enough?

return handlePartitionKeyRangeGoneException((PartitionKeyRangeGoneException) exception);
}

throw new IllegalStateException("Invalid exception type", exception);
Expand All @@ -316,6 +319,18 @@ private Pair<Mono<ShouldRetryResult>, Boolean> handlePartitionKeyIsSplittingExce
this.request.forcePartitionKeyRangeRefresh = true;
return Pair.of(null, false);
}

private Pair<Mono<ShouldRetryResult>, Boolean> handlePartitionKeyRangeGoneException(PartitionKeyRangeGoneException exception) {
// PartitionKeyRangeGoneException is generally treated as non-retriable, but when it is thrown while resolving
// addresses in direct mode it typically indicates stale routing/partition state; clear the cached target and
// force a routing-map (partition key range) refresh to allow the request to be re-routed.
this.request.requestContext.resolvedPartitionKeyRange = null;
this.request.requestContext.quorumSelectedLSN = -1;
this.request.requestContext.quorumSelectedStoreResponse = null;
logger.debug("Received partition key range gone exception, will retry, {}", exception.toString());
this.request.forcePartitionKeyRangeRefresh = true;
return Pair.of(null, false);
Comment thread
arnabnandy7 marked this conversation as resolved.
}
Comment thread
arnabnandy7 marked this conversation as resolved.
}

}
Loading