Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Move hedging on top of ClientRetryPolicy #36508

Merged
merged 55 commits into from
Sep 15, 2023
Merged

Move hedging on top of ClientRetryPolicy #36508

merged 55 commits into from
Sep 15, 2023

Conversation

FabianMeiswinkel
Copy link
Member

@FabianMeiswinkel FabianMeiswinkel commented Aug 24, 2023 •
edited
Loading

Description

This PR addresses a few issues identified with E2ETimeout and Availability strategy. The current approach to inject the hedging across regions in ReplicatedResourceClient has a few issues:

  • For an operation like readItem the ClientRetryPolicy controlling cross-regional retries sits on top of the ReplicatedResourceClient - which would lead to possibly starting the hedging multiple times (initial attempt and every time the client retry policy retries cross region)
  • The hedging in ReplicatedResourceClient uses Mono.firstWithValue and starts a Composite Mono with Monos for each region - this means if requests fail - the operator would wait for all of them to succeed/fail. In situations where errors are expected - like a legit 404/0 a 404-Conflict etc. this would mean waiting for all hedging attempts to finish before bubbling up the NotFoundExceptions - this would result in unnecessarily high latency for 404/0, 409 or 412.
  • ReplicatedResourceClient would add any Request/Response to store results in ComsosDiagnostics - which means we would end-up with one CosmosDiagnostics instances containing all request/response pairs for all hedging attempts. The initial idea was instead to add multiple CosmosDIagnostics instances to the CosmosDiagnosticsContext - one for each hedging attempt. This would simplify debugging - and also diagnostics and metrics enhancement are designed around this.

To address these challenges this PR is moving the hedging to the RxDocumentClientImpl - so, on top of ClientRetryPolicy. This PR only addresses the changes for point operations - not yet queries

The unit tests added will use a Multi-Region, Multi-Master account to test the availability strategy on a real account.

Changelog should be updated with the query PR

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.

jeet1995 and others added 16 commits August 9, 2023 09:01
@jeet1995 @kushagraThapar
Updated read hang grace and cancellation count threshold (Azure#36280)
983e812 
* Updated read hang grace and cancellation count threshold

* Updated CHANGELOG.md

* Updated CHANGELOG.md

* Updated CHANGELOG.md
@kushagraThapar
Updated release versions to 4.48.1 for azure-cosmos (Azure#36291)
a15ac3e 
* Updated release versions to 4.48.1 for azure-cosmos

* Updated release notes
@azure-sdk
Increment package versions for cosmos releases (Azure#36301)
45ad3cd
@jeet1995
Preliminary handling of exceptions returned from all regions when Ava…
905619c 
…ilabilityStrategy is enabled.
@FabianMeiswinkel
Refactoring the AvailabilityStrategyNoSuchElementFix
ba8c785
@FabianMeiswinkel
Refactoring the AvailabilityStrategyNoSuchElementFix (Azure#36468)
adcad5e
@FabianMeiswinkel
Reacting to code review comments
bd707a9
@FabianMeiswinkel
Merge branch 'AvailabilityStrategyAllRegionExceptionFix' into users/f…
d69ae13 
…abianm/AvailabilityStrategyAllRegionExceptionFix
@FabianMeiswinkel
Merge branch 'AvailabilityStrategyAllRegionExceptionFix' of https://g…
44e191c 
…ithub.com/Azure/azure-sdk-for-java into users/fabianm/AvailabilityStrategyAllRegionExceptionFix
@FabianMeiswinkel
Merge branch 'users/fabianm/AvailabilityStrategyAllRegionExceptionFix…
72c84ff 
…' of https://github.com/FabianMeiswinkel/azure-sdk-for-java into users/fabianm/AvailabilityStrategyAllRegionExceptionFix
@FabianMeiswinkel
Users/fabianm/availability strategy all region exception fix (Azure#3…
d11f987 
…6479)

* Refactoring the AvailabilityStrategyNoSuchElementFix

* Reacting to code review comments
@FabianMeiswinkel
Merge branch 'AvailabilityStrategyAllRegionExceptionFix' of https://g…
b24338c 
…ithub.com/Azure/azure-sdk-for-java into users/fabianm/AvailabilityStrategyAllRegionExceptionFix
@FabianMeiswinkel
Snapshot
9a3cf54
@FabianMeiswinkel
Update ReplicatedResourceClient.java
414a3ed
@FabianMeiswinkel
Iterating on implementation
bbb7f88
@FabianMeiswinkel
Merge branch 'main' of https://github.com/Azure/azure-sdk-for-java in…
8ba605e 
…to users/fabianm/AvailabilityStrategyAllRegionExceptionFix
@FabianMeiswinkel FabianMeiswinkel marked this pull request as draft August 24, 2023 22:27
xinlian12
xinlian12 approved these changes Sep 10, 2023
View reviewed changes
Copy link
Member

@xinlian12 xinlian12 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Great changes, thanks a lot :)

@FabianMeiswinkel
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FabianMeiswinkel
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FabianMeiswinkel
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FabianMeiswinkel
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@xinlian12
Copy link
Member

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FabianMeiswinkel
Copy link
Member Author

/azp run java - cosmos - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@FabianMeiswinkel FabianMeiswinkel merged commit 7aa44d6 into Azure:main Sep 15, 2023
59 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbhaskar mbhaskar mbhaskar left review comments

@jeet1995 jeet1995 jeet1995 left review comments

@xinlian12 xinlian12 xinlian12 approved these changes

@kushagraThapar kushagraThapar Awaiting requested review from kushagraThapar kushagraThapar is a code owner

@kirankumarkolli kirankumarkolli Awaiting requested review from kirankumarkolli kirankumarkolli is a code owner

@milismsft milismsft Awaiting requested review from milismsft milismsft is a code owner

@aayush3011 aayush3011 Awaiting requested review from aayush3011 aayush3011 is a code owner

@simorenoh simorenoh Awaiting requested review from simorenoh simorenoh is a code owner

@Pilchie Pilchie Awaiting requested review from Pilchie Pilchie is a code owner

@mssfang mssfang Awaiting requested review from mssfang mssfang is a code owner

@JonathanGiles JonathanGiles Awaiting requested review from JonathanGiles JonathanGiles is a code owner

Assignees
No one assigned
Labels
Cosmos
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@FabianMeiswinkel @azure-sdk @xinlian12 @mbhaskar @jeet1995 @kushagraThapar