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.

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

retries: Early commit can break transparent retries #10011

Closed
ejona86 opened this issue Apr 4, 2023 · 0 comments · Fixed by #10066
Closed

retries: Early commit can break transparent retries #10011

ejona86 opened this issue Apr 4, 2023 · 0 comments · Fixed by #10066
Assignees
Labels
Milestone

Comments

@ejona86
Copy link
Member

ejona86 commented Apr 4, 2023

There is the transparent retry condition:

if (rpcProgress == RpcProgress.MISCARRIED
|| (rpcProgress == RpcProgress.REFUSED
&& noMoreTransparentRetry.compareAndSet(false, true))) {

And then within the transparent retry handling, it surprisingly looks at the hedging/retry policy:

if (retryPolicy == null || retryPolicy.maxAttempts == 1) {

That's just... wrong. There should never have been be any early commit in the transparent retry case based on the retry policy. I could understand more if this was in the normal retry/hedging flows, but those don't have the early commit. And it is good they don't, as it would break transparent retries.

Seems we should just remove all the early commits.

It may be fair to have @temawi or @larry-safran work on this, as it is easy/surgical and @YifeiZhuang reviews. Y'all can discuss amongst yoruselves.

@ejona86 ejona86 added this to the 1.55 milestone Apr 4, 2023
@ejona86 ejona86 added the bug label Apr 5, 2023
larry-safran added a commit to larry-safran/grpc-java that referenced this issue Apr 18, 2023
larry-safran added a commit to larry-safran/grpc-java that referenced this issue Apr 19, 2023
The DECRYPTION_FAILURE_RE matcher had an exclamation point that was dropped from the related error message between Java 11 and 19.

Fixes grpc#10011
larry-safran added a commit that referenced this issue Apr 19, 2023
…ng. (#10066)

* retries:Remove early commit for transparent retries when no retries or no hedging remain.

Fixes #10011
larry-safran added a commit to larry-safran/grpc-java that referenced this issue Apr 19, 2023
…ng. (grpc#10066)

* retries:Remove early commit for transparent retries when no retries or no hedging remain.

Fixes grpc#10011
larry-safran added a commit to larry-safran/grpc-java that referenced this issue Apr 19, 2023
…ng. (grpc#10066)

* retries:Remove early commit for transparent retries when no retries or no hedging remain.

Fixes grpc#10011
larry-safran added a commit to larry-safran/grpc-java that referenced this issue Apr 19, 2023
…ng. (grpc#10066)

* retries:Remove early commit for transparent retries when no retries or no hedging remain.

Fixes grpc#10011
larry-safran added a commit that referenced this issue Apr 19, 2023
…ng. (#10066) (#10082)

* retries:Remove early commit for transparent retries when no retries or no hedging remain.

Fixes #10011
larry-safran added a commit that referenced this issue Apr 19, 2023
…ng. (#10066) (#10081)

* retries:Remove early commit for transparent retries when no retries or no hedging remain.

Fixes #10011
larry-safran added a commit that referenced this issue Apr 20, 2023
The DECRYPTION_FAILURE_RE matcher had an exclamation point that was dropped from the related error message between Java 11 and 19.

Fixes #10011
larry-safran added a commit that referenced this issue Apr 21, 2023
…ng. (#10066) (#10080)

* retries:Remove early commit for transparent retries when no retries or no hedging remain.

Fixes #10011
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants