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

[3.5] backport for fix retry requests when receiving ErrGPRCNotSupportedForLearner #17641

Merged
merged 1 commit into from
Apr 4, 2024

Conversation

sheyt0
Copy link
Contributor

@sheyt0 sheyt0 commented Mar 25, 2024

Backport of PR #12985

@k8s-ci-robot
Copy link

Hi @sheyt0. Thanks for your PR.

I'm waiting for a etcd-io member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sheyt0 sheyt0 mentioned this pull request Mar 25, 2024
3 tasks
@ivanvc
Copy link
Member

ivanvc commented Mar 25, 2024

Hi @sheyt0, thanks for the pull request. Please ensure your commit is signed so the Developer Certificate of Origin (DCO) check passes, i.e:

git rebase HEAD~1 --signoff
git push --force

/ok-to-test

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM - Thanks @sheyt0

@serathius
Copy link
Member

/cc @ahrtr

@k8s-ci-robot k8s-ci-robot requested a review from ahrtr March 27, 2024 18:51
@ahrtr
Copy link
Member

ahrtr commented Mar 27, 2024

I do not see any big problem on this PR. But for safety, I suggest that we do not change the error code in stable release.

But I agree that we should backport the change in isSafeRetry, we do need to instruct the client to retry when it receives an ErrGPRCNotSupportedForLearner error and len(endpoints) > 1.

@sheyt0
Copy link
Contributor Author

sheyt0 commented Mar 29, 2024

Who decides what form a fix should take?
I can fix the backport by only including the change for replays for learner. But I expect at least some comments on this matter.

@ahrtr
Copy link
Member

ahrtr commented Mar 31, 2024

@sheyt0 please follow my comment above #17641 (comment) to update this PR.

  • We do need to update isSafeRetry as this PR already did
  • Regarding the error code changes, as I mentioned above, I did not see any big problem. But it changes the protocol between client and server, so I suggest not to change them in stable releases.

@sheyt0 sheyt0 changed the title [3.5] backport for fix not retryable error codes [3.5] backport for fix retry requests when receiving ErrGPRCNotSupportedForLearner Apr 1, 2024
@ahrtr
Copy link
Member

ahrtr commented Apr 1, 2024

@sheyt0 can you also please backport the fix to 3.4?

And also update the changelog for both 3.5 and 3.4 in a separate PR?

@ahrtr ahrtr merged commit f5092bc into etcd-io:release-3.5 Apr 4, 2024
18 checks passed
@sheyt0 sheyt0 deleted the v3.5.13 branch April 4, 2024 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

8 participants