-
Notifications
You must be signed in to change notification settings - Fork 525
Barrier Requests: Adds Integration tests for retry logic for 410/ Lease Not Found (1022) exceptions #5469
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
Conversation
| // Handle barrier (HEAD) requests | ||
| if (request.OperationType == OperationType.Head) | ||
| { | ||
| barrierRequestCount++; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For the purpose of differentiating the first and rest requests, it would be simpler to use a boolean:
bool firstRequest = true;
CosmosClientOptions clientOptions //...
{
TransportClientHandlerFactory // ...
interceptorAfterResult: // ...
{
// ...
if (request.OperationType == OperationType.Head)
{
// ...
if (firstRequest)
{
// ...
firstRequest = false;
} else
{
// ...
}
}
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if it is not only about 2 requests, it would be meaningful to fail on barrierRequestCount > 2 in the if else ladder and fail fast
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The separate boolean approach might not work here because these tests validate different retry scenarios with varying expected behaviors - some tests expect exactly 2 barrier requests (success case), while others specifically test the SDK's retry exhaustion behavior that can generate up to 40 attempts.
...t.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BarrierRequestReplicaRetryTests.cs
Outdated
Show resolved
Hide resolved
...t.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BarrierRequestReplicaRetryTests.cs
Outdated
Show resolved
Hide resolved
kirankumarkolli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two more scenarios to cover
- 410/1022: From primary results in failure for Writes
- 410/1022: From primary results in cross-region retry and succceds
|
Please also update the description |
...t.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BarrierRequestReplicaRetryTests.cs
Outdated
Show resolved
Hide resolved
...t.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BarrierRequestReplicaRetryTests.cs
Outdated
Show resolved
Hide resolved
...t.Azure.Cosmos/tests/Microsoft.Azure.Cosmos.EmulatorTests/BarrierRequestReplicaRetryTests.cs
Outdated
Show resolved
Hide resolved
…re/azure-cosmos-dotnet-v3 into users/aavasthy/barrierupdate
Pull Request Template
Description
If a barrier request receives a 410/1022 from the backend for any given replica (quorum replicas in case of reads), then the SDK should attempt to retry the barrier request in the primary replica. If the primary replica responds with a 410/1022, then the entire Read or Write operation should bail-out/ fail and Reads/ Writes (either PPAF enabled on SM or in MM) should be retried on the next preferred region.
This PR adds integration tests for testing end to end flow with direct package changes.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #5383