-
Notifications
You must be signed in to change notification settings - Fork 2.2k
Fix direct retry handling for partition key range gone #49613
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.
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
6cc81af
e2ec601
ad65758
5c965ce
7bad7f2
3a5f527
2e2fe7b
1fe615c
b0c0228
012dede
7ee385c
ceae70b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -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) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 I agree the PR should prove that path with FaultInjection instead of only unit tests. I see
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @FabianMeiswinkel I tightened this so The retry path is now opt-in only for address-resolution-created exceptions:
I also added/updated unit coverage in 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); | ||
|
|
@@ -316,6 +319,15 @@ private Pair<Mono<ShouldRetryResult>, Boolean> handlePartitionKeyIsSplittingExce | |
| this.request.forcePartitionKeyRangeRefresh = true; | ||
| return Pair.of(null, false); | ||
| } | ||
|
|
||
| private Pair<Mono<ShouldRetryResult>, Boolean> handlePartitionKeyRangeGoneException(PartitionKeyRangeGoneException exception) { | ||
| 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); | ||
|
arnabnandy7 marked this conversation as resolved.
|
||
| } | ||
|
arnabnandy7 marked this conversation as resolved.
|
||
| } | ||
|
|
||
| } | ||
Uh oh!
There was an error while loading. Please reload this page.