Fix setRetryOptions merging initialInterval into congestionInitialInterval#2956
Open
samarth70 wants to merge 1 commit into
Open
Fix setRetryOptions merging initialInterval into congestionInitialInterval#2956samarth70 wants to merge 1 commit into
samarth70 wants to merge 1 commit into
Conversation
…erval RpcRetryOptions.Builder.setRetryOptions merged every field from its own counterpart except congestionInitialInterval, which read o.getInitialInterval(). The source's congestionInitialInterval was therefore never read, and the value was overwritten by the regular initial interval. congestionInitialInterval is the backoff applied to RESOURCE_EXHAUSTED (used by GrpcSyncRetryer/GrpcAsyncRetryer via BackoffThrottler) and defaults to 10x the regular interval (1000ms vs 100ms). Merging collapsed that margin, so a client would retry an already-congested server far faster than intended - the opposite of what the setting exists for. Read o.getCongestionInitialInterval() and add a regression test asserting a user-set congestion interval survives setRetryOptions.
This file contains hidden or 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
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.
Problem
RpcRetryOptions.Builder.setRetryOptionsmerges every field from its own counterpart — exceptcongestionInitialInterval, which reads the source'sinitialInterval:Two consequences: the source's
congestionInitialIntervalis never read, and the value is silently overwritten by the regular initial interval.Why it matters
congestionInitialIntervalis the backoff applied to gRPCRESOURCE_EXHAUSTED(viaBackoffThrottler, used byGrpcSyncRetryer/GrpcAsyncRetryer), and it is deliberately defaulted 10x higher than the regular interval —DefaultStubServiceOperationRpcRetryOptionssetsINITIAL_INTERVAL=100msvsCONGESTION_INITIAL_INTERVAL=1000ms.Merging collapses that margin, so a client ends up retrying an already-congested server faster than intended — the opposite of the field's purpose. #1465, which introduced it, states the rationale directly: "Aggressive retry will worsen the situation and could lead to more serious outage."
Fix
Read
o.getCongestionInitialInterval().Test
Adds
RpcRetryOptionsTest.setRetryOptionsMergesCongestionInitialInterval: builds a source with a 200ms initial / 7s congestion interval, merges it viasetRetryOptions, and asserts both survive.Against unmodified source it fails with:
and passes with the fix.
temporal-serviceclientis pure logic — the test needs no server or docker (./gradlew :temporal-serviceclient:test --tests "io.temporal.serviceclient.RpcRetryOptionsTest"). Spotless check clean.