Client Retry Policy: Adds HTTP timeouts with request-level cross-region retry #32450

NaluTripician · 2022-12-05T21:06:52Z

Description

Based on issue created here.

Java V4 needs to add cross-region retries for the following combination of request types:

Type	Read/Write
Data plane	Read
~~Data plane~~	~~Write~~
Metadata	Read
Query Plan	Read (it is always read)

This is a mirror of this issue and PR on the .NET SDK and resolves Issue #31367.

On the Java side, after retries are expended on the WebRetryPolicy, It will retry through the ClientRetryPolicy. In the shouldRetry method upon receiving a Network Failure where the Gateway Endpoint timed out (Sub-Status Code 10002), the SDK will now detect if it is one of the above cases and attempt to see if it can failover to another region.

Changes also include behavior for Query Plan operations. Because query plan retries on endpoint timeouts are now handled with data plane reads~+writes~ and metadata reads, this behavior change needed to be reflected in the tests.

Handling of dataplane writes are not included in this PR due to concern with the safety of retrying them. Once more investigation is conducted a new PR will be issued with this changes or this PR will be updated with the results.

All SDK Contribution checklist:

The pull request does not introduce [breaking changes]
CHANGELOG is updated for new features, bug fixes or other significant changes.
I have read the contribution guidelines.

General Guidelines and Best Practices

Title of the pull request is clear and informative.
There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

Pull request includes test coverage for the included changes.

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java

FabianMeiswinkel

LGTM except for two minor suggestions.

FabianMeiswinkel

Please also add the changelog entries

Co-authored-by: Fabian Meiswinkel <[email protected]>

xinlian12 · 2022-12-06T20:02:31Z

/azp run java - cosmos - tests

azure-pipelines · 2022-12-06T20:02:45Z

Azure Pipelines successfully started running 1 pipeline(s).

xinlian12 · 2022-12-06T22:12:11Z

Failed test: conflictCustomLWW (known flaky test)

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java

sdk/cosmos/azure-cosmos/CHANGELOG.md

xinlian12 · 2022-12-06T22:43:10Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java

+        }
+
+        //Data Plane Read & Write
+        if(!isMetaDataRequest && !request.isAddressRefresh()) {


write operation also retriable? (how we know the request is not reached to server?)

Yes, data plane writes (meta data writes are not) operations are retriable and should be handled in this case. I believe that because we are getting an error code the request never reached the server.

Good catch Annie - I missed this

@NaluTripician - no, write operations are not in general retriable - only when we can be sure that the request has never been processed. So, when we get certain error codes form the service (410/0 sent form the service)or when we get a timeout trying to establish a connection we know that the request has never been processed and retry is idempotent/safe. But a request timeout after we sent the request on the wire means we simply don't know whether the request was ever processed in the service - so, a retry would not be safe. Usually we capture state of whether request has been flushed to the network yet and also whether a 410 comes form the service or is clinet-side generated to make the call whether writes can be retried. Sounds like above logic might be too aggressive with retries - can you please double-check?

xinlian12 · 2022-12-08T18:26:45Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java

        //Data Plane Read & Write
-        if(!isMetaDataRequest && !request.isAddressRefresh()) {
+        if(!isMetaDataRequest && !request.isAddressRefresh()
+            && (request.isReadOnly() || !BridgeInternal.hasSendingRequestStarted(clientException))) {


I see two options here:

for http timeout exception, usually it is an indicated that SDK has sent the request, else SDK will translate those exceptions into GATEWAY_ENDPOINT_UNAVAILABLE. so we could remove the write operation completely here I think

Using the hasSendingRequestStarted - this flag has only been wired up in direct rntbd layer, but not for gateway requests, so we will need to wire it up for gateway requests as well. -- that is also why the tests you have now still succeeded after the change

...cosmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/ClientRetryPolicyTest.java

xinlian12

LGTM, thanks

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java

…ntation/ClientRetryPolicy.java Co-authored-by: Kushagra Thapar <[email protected]>

…/github.com/Azure/azure-sdk-for-java into users/nalutripician/HttpTimeoutGatewayRetry

NaluTripician · 2022-12-13T14:33:31Z

/azp run java - cosmos - tests

azure-pipelines · 2022-12-13T14:33:45Z

Azure Pipelines successfully started running 1 pipeline(s).

simorenoh · 2022-12-15T22:15:01Z

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java

        return this.throttlingRetry.shouldRetry(e);
    }

+      private boolean canGatewayRequestFailoverOnTimeout(RxDocumentServiceRequest request, CosmosException clientException) {


I see the method takes in clientException but doesn't use it anywhere - is this intended?

mirror of .net fix - initial commit

acb39ad

NaluTripician requested review from FabianMeiswinkel, Pilchie, aayush3011, jeet1995, kirankumarkolli, kushagraThapar, milismsft, simorenoh and xinlian12 as code owners December 5, 2022 21:06

ghost added the Cosmos label Dec 5, 2022

FabianMeiswinkel reviewed Dec 5, 2022

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java Outdated Show resolved Hide resolved

FabianMeiswinkel reviewed Dec 5, 2022

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java Outdated Show resolved Hide resolved

FabianMeiswinkel approved these changes Dec 5, 2022

View reviewed changes

FabianMeiswinkel reviewed Dec 5, 2022

View reviewed changes

NaluTripician and others added 4 commits December 6, 2022 10:21

nits

d7637a0

Co-authored-by: Fabian Meiswinkel <[email protected]>

nits

ec9354c

Co-authored-by: Fabian Meiswinkel <[email protected]>

Merge branch 'main' into users/nalutripician/HttpTimeoutGatewayRetry

fc15d16

updated changelog, fixes to tests/address refresh beahvior

899d732

xinlian12 reviewed Dec 6, 2022

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java Outdated Show resolved Hide resolved

xinlian12 reviewed Dec 6, 2022

View reviewed changes

sdk/cosmos/azure-cosmos/CHANGELOG.md Outdated Show resolved Hide resolved

xinlian12 reviewed Dec 6, 2022

View reviewed changes

NaluTripician added 2 commits December 7, 2022 09:15

removal of double handling of queryplan, improved changelog message

d68c92b

added check to see if write requests are safe to retry

c53cc0d

xinlian12 reviewed Dec 8, 2022

View reviewed changes

NaluTripician added 2 commits December 8, 2022 14:22

removed handeling of data plane writes

eadc967

removed extra test

d2a6687

xinlian12 reviewed Dec 8, 2022

View reviewed changes

...cosmos/azure-cosmos/src/test/java/com/azure/cosmos/implementation/ClientRetryPolicyTest.java Outdated Show resolved Hide resolved

xinlian12 approved these changes Dec 8, 2022

View reviewed changes

xinlian12 reviewed Dec 8, 2022

View reviewed changes

sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/ClientRetryPolicy.java Show resolved Hide resolved

kushagraThapar reviewed Dec 8, 2022

View reviewed changes

NaluTripician and others added 3 commits December 9, 2022 12:02

Update sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/impleme…

50dbf61

…ntation/ClientRetryPolicy.java Co-authored-by: Kushagra Thapar <[email protected]>

added testing for data plane writes, simplified query plan check

c326681

Merge branch 'users/nalutripician/HttpTimeoutGatewayRetry' of https:/…

d523cb0

…/github.com/Azure/azure-sdk-for-java into users/nalutripician/HttpTimeoutGatewayRetry

NaluTripician mentioned this pull request Dec 9, 2022

[Tracking] Safety of cross regional retries on data plane writes that timeout on Gateway mode. #32554

Closed

NaluTripician and others added 2 commits December 12, 2022 12:14

nits

b4a55b4

Merge branch 'main' into users/nalutripician/HttpTimeoutGatewayRetry

67c3964

kushagraThapar merged commit fa0869d into main Dec 14, 2022

kushagraThapar deleted the users/nalutripician/HttpTimeoutGatewayRetry branch December 14, 2022 16:50

simorenoh reviewed Dec 15, 2022

View reviewed changes

Client Retry Policy: Adds HTTP timeouts with request-level cross-region retry #32450

Client Retry Policy: Adds HTTP timeouts with request-level cross-region retry #32450

Uh oh!

Conversation

NaluTripician commented Dec 5, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

All SDK Contribution checklist:

General Guidelines and Best Practices

Testing Guidelines

Uh oh!

Uh oh!

Uh oh!

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

Uh oh!

FabianMeiswinkel left a comment

Choose a reason for hiding this comment

Uh oh!

xinlian12 commented Dec 6, 2022

Uh oh!

azure-pipelines bot commented Dec 6, 2022

Uh oh!

xinlian12 commented Dec 6, 2022

Uh oh!

Uh oh!

Uh oh!

xinlian12 Dec 6, 2022

Choose a reason for hiding this comment

Uh oh!

NaluTripician Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

FabianMeiswinkel Dec 7, 2022

Choose a reason for hiding this comment

Uh oh!

xinlian12 Dec 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xinlian12 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

NaluTripician commented Dec 13, 2022

Uh oh!

azure-pipelines bot commented Dec 13, 2022

Uh oh!

simorenoh Dec 15, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

NaluTripician commented Dec 5, 2022 •

edited

Loading

xinlian12 Dec 8, 2022 •

edited

Loading