ClientRetryPolicy: Adds Cross Regional Retry Logic on 429/3092 #4691
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…try_on_3092_and_1022
kundadebdatta
added
Do Not Review
kundadebdatta
requested review from
khdang,
sboshra,
adityasa,
neildsh,
kirankumarkolli,
FabianMeiswinkel and
kirillg
as code owners
kundadebdatta
commented
Sep 13, 2024
auto-merge was automatically disabled
September 14, 2024 05:01
Pull request was converted to draft
kirankumarkolli
previously approved these changes
Sep 17, 2024
kirankumarkolli
approved these changes
Sep 17, 2024
NaluTripician
approved these changes
Sep 17, 2024
microsoft-github-policy-service
bot
deleted the
users/kundadebdatta/4656_cross_regional_retry_on_429_3092
branch
NaluTripician
pushed a commit
that referenced
this pull request
Sep 18, 2024
* Initial code changes to throw 503 on 429/3092. * Updated client retry policy. Added more tests to cover 429/3092. * Code changes to update direct package version. Updating the tests. * Code changes to refactor client retry policy. * Minor code cleanup. * Reverting the direct version bump up change. * Code changes to address some of the review comments. * Code changes to move failover logic in client retry policy. * Minor code clean up. * Code changes to clean up some cosmetic items. * Further clean up. * Code changes to address review comments. * Minor refactor to address cosmetic update. * Code changes to address cosmetic review comment.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Pull Request Template
Description
This PR is covering the below CRI scenario in general:
Background:
For
429/3092 (Request Throttled/ SystemResourceUnavailable), we would only want to short-circuit this to retry cross-region quickly when we can actually retry cross region (not a write in single master) - and for reads only when it is returned from all replica in a region. The latter is extremely unlikely - so, I would probably compromise on a pragmatic approach and simply short-circuit by mapping429/3092to a 503 for write operations when multi-master is enabled (and there is >1 write regions).That way we get the cross-region retry in the scenario where this is most critical.
Type of change
Please delete options that are not relevant.
Closing issues
To automatically close an issue: closes #4656