Internal: Fixed exception handling around GoneException and RequestTimeoutException by FabianMeiswinkel · Pull Request #15735 · Azure/azure-sdk-for-java

FabianMeiswinkel · 2020-09-25T23:02:52Z

Fixing a couple of issues with the exception handling in the availability stack:

Write-requests which have been sent to the service should not be retried when they fail with GoneException
Read-requests resulting in Request timeouts (when the time between sending the request and waiting for response extends the request timeout duration or service returns 408) should be retried
Admission control incorrectly was throwing a RequestTimeoutException -should have been a GoneException instead

Closes #15363
Closes #15305
Closes #15304

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

FabianMeiswinkel · 2020-09-25T23:03:58Z

Looking into options to add some additional test coverage (for admission control and the request timeout exception to gone exception mapping)

...s/src/main/java/com/azure/cosmos/implementation/directconnectivity/RntbdTransportClient.java

...c/main/java/com/azure/cosmos/implementation/directconnectivity/rntbd/RntbdRequestRecord.java

...ain/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java

moderakh

address refresh is done as part of the retry.

If no one retries who is going to refresh the address caches when request.requestContext.forceRefreshAddressCache set to true?

FabianMeiswinkel · 2020-09-29T02:17:12Z

/azp run java - cosmos - tests

azure-pipelines · 2020-09-29T02:17:23Z

Azure Pipelines successfully started running 1 pipeline(s).

...mos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentServiceRequest.java

...ain/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java

FabianMeiswinkel · 2020-09-29T10:59:05Z

/azp run java - cosmos - tests

azure-pipelines · 2020-09-29T10:59:15Z

Azure Pipelines successfully started running 1 pipeline(s).

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

FabianMeiswinkel · 2020-09-29T11:52:09Z

/azp run java - cosmos - tests

azure-pipelines · 2020-09-29T11:52:19Z

Azure Pipelines successfully started running 1 pipeline(s).

...ain/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java

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

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

FabianMeiswinkel · 2020-09-30T12:40:26Z

/azp run java - cosmos - tests

azure-pipelines · 2020-09-30T12:40:37Z

Azure Pipelines successfully started running 1 pipeline(s).

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

moderakh · 2020-09-30T18:58:11Z

/azp run java - cosmos - tests

azure-pipelines · 2020-09-30T18:58:21Z

Azure Pipelines successfully started running 1 pipeline(s).

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

moderakh

you don't need to wait on address refresh cache when write request result in gone.

you could instead trigger an address cache refresh in background and return the write response immediately.

See this as example:
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/ConsistencyWriter.java#L365-L373

moderakh · 2020-09-30T19:55:32Z

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

+                        shouldRetryResult.policyArg.getValue0() != null &&
+                        shouldRetryResult.policyArg.getValue0()) {
+
+                        return addressSelector.resolveAddressesAsync(


this is for refreshing address cache on a Gone Exception for write request.

With this approach the write response will get blocked till the unrelated address resolution completes.

you could instead trigger an address cache refresh in background and return the write response immediately.

See this as example:
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/ConsistencyWriter.java#L365-L373

Fixed - good catch. Thanks!

...c/main/java/com/azure/cosmos/implementation/directconnectivity/rntbd/RntbdRequestRecord.java

moderakh

for failed write, please do the address cache refresh in background.

other than that LGTM.
https://github.com/Azure/azure-sdk-for-java/blob/master/sdk/cosmos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/directconnectivity/ConsistencyWriter.java#L365-L373

FabianMeiswinkel · 2020-09-30T23:43:51Z

/azp run java - cosmos - tests

azure-pipelines · 2020-09-30T23:44:01Z

Azure Pipelines successfully started running 1 pipeline(s).

kushagraThapar

Overall looks good to me, some minor suggestions and one blocking comment to add Microsoft copyright header to the new file.

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

.../main/java/com/azure/cosmos/implementation/directconnectivity/rntbd/RntbdRequestManager.java

...c/main/java/com/azure/cosmos/implementation/directconnectivity/rntbd/RntbdRequestRecord.java

...azure/cosmos/implementation/directconnectivity/ReplicatedResourceClientGoneForWriteTest.java

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

kushagraThapar

Thanks @FabianMeiswinkel looks good to me.

FabianMeiswinkel · 2020-10-01T00:39:28Z

/azp run java - cosmos - tests

azure-pipelines · 2020-10-01T00:39:38Z

Azure Pipelines successfully started running 1 pipeline(s).

FabianMeiswinkel · 2020-10-01T20:01:59Z

/check-enforcer evaluate

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

…which have been sent

FabianMeiswinkel · 2020-10-01T23:08:48Z

/azp run java - cosmos - tests

azure-pipelines · 2020-10-01T23:08:59Z

Azure Pipelines successfully started running 1 pipeline(s).

FabianMeiswinkel added 3 commits September 25, 2020 22:13

Fixing exception handling for Gone and Request timeouts

e3f011f

Adding basic unit tests

758a859

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

f0c6809

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

FabianMeiswinkel requested review from David-Noble-at-work, kirankumarkolli, kushagraThapar, mbhaskar, milismsft, moderakh, simplynaveen20 and xinlian12 as code owners September 25, 2020 23:02

ghost added the Cosmos label Sep 25, 2020

moderakh suggested changes Sep 28, 2020

View reviewed changes

moderakh self-requested a review September 28, 2020 17:22

moderakh reviewed Sep 28, 2020

View reviewed changes

FabianMeiswinkel added 2 commits September 29, 2020 01:27

Really enforcing AddressRefresh after failing Gone for writes

07ac3ba

Fixing unit test regression

585dea7

kirankumarkolli reviewed Sep 29, 2020

View reviewed changes

...mos/azure-cosmos/src/main/java/com/azure/cosmos/implementation/RxDocumentServiceRequest.java Show resolved Hide resolved

kirankumarkolli reviewed Sep 29, 2020

View reviewed changes

...ain/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java Outdated Show resolved Hide resolved

kirankumarkolli approved these changes Sep 29, 2020

View reviewed changes

Changing log-level to warn if we don't retry

9f985dc

FabianMeiswinkel requested a review from moderakh September 29, 2020 10:43

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

21ff088

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

xinlian12 reviewed Sep 29, 2020

View reviewed changes

...ain/java/com/azure/cosmos/implementation/directconnectivity/GoneAndRetryWithRetryPolicy.java Show resolved Hide resolved

xinlian12 reviewed Sep 29, 2020

View reviewed changes

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

FabianMeiswinkel added 2 commits September 29, 2020 21:59

Update log4j2.properties

b3368ad

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

0ac49b1

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

FabianMeiswinkel added 3 commits September 30, 2020 18:10

Fixing unit test flakiness

97f9527

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

157db1c

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

Merging with Mo's Diagnostics PR

9cefd30

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

41ecf26

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

moderakh reviewed Sep 30, 2020

View reviewed changes

moderakh self-requested a review September 30, 2020 20:44

moderakh approved these changes Sep 30, 2020

View reviewed changes

Moving the address refresh to a background task for writes hitting 410

45cfea1

kushagraThapar requested changes Oct 1, 2020

View reviewed changes

FabianMeiswinkel added 2 commits October 1, 2020 00:28

Addressed Kushagra's PR comments

6ad514f

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

4fac63e

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

kushagraThapar approved these changes Oct 1, 2020

View reviewed changes

FabianMeiswinkel added 2 commits October 1, 2020 20:04

Merge branch 'master' of https://github.com/Azure/azure-sdk-for-java …

0e14727

…into users/fabianm/GoneAndRequestTimeoutExceptionHandling

Addressing benchmark flakiness introduced due to not retrying writes …

6f0b142

…which have been sent

FabianMeiswinkel merged commit 187d9e9 into Azure:master Oct 2, 2020

Conversation

FabianMeiswinkel commented Sep 25, 2020

Uh oh!

FabianMeiswinkel commented Sep 25, 2020

Uh oh!

Uh oh!

Uh oh!

Uh oh!

moderakh left a comment • 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!

FabianMeiswinkel commented Sep 29, 2020

Uh oh!

azure-pipelines bot commented Sep 29, 2020

Uh oh!

Uh oh!

Uh oh!

FabianMeiswinkel commented Sep 29, 2020

Uh oh!

azure-pipelines bot commented Sep 29, 2020

Uh oh!

FabianMeiswinkel commented Sep 29, 2020

Uh oh!

azure-pipelines bot commented Sep 29, 2020

Uh oh!

Uh oh!

Uh oh!

FabianMeiswinkel commented Sep 30, 2020

Uh oh!

azure-pipelines bot commented Sep 30, 2020

Uh oh!

moderakh commented Sep 30, 2020

Uh oh!

azure-pipelines bot commented Sep 30, 2020

Uh oh!

moderakh left a comment

Choose a reason for hiding this comment

Uh oh!

moderakh Sep 30, 2020 • 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!

FabianMeiswinkel Sep 30, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

moderakh left a comment

Choose a reason for hiding this comment

Uh oh!

FabianMeiswinkel commented Sep 30, 2020

Uh oh!

azure-pipelines bot commented Sep 30, 2020

Uh oh!

kushagraThapar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kushagraThapar left a comment

Choose a reason for hiding this comment

Uh oh!

FabianMeiswinkel commented Oct 1, 2020

Uh oh!

azure-pipelines bot commented Oct 1, 2020

Uh oh!

FabianMeiswinkel commented Oct 1, 2020

Uh oh!

FabianMeiswinkel commented Oct 1, 2020

Uh oh!

azure-pipelines bot commented Oct 1, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

moderakh left a comment •

edited

Loading

moderakh Sep 30, 2020 •

edited

Loading