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

etcdserver: skip when detect a removed peer #17518

Merged
merged 1 commit into from
Mar 5, 2024

Conversation

datbeohbbh
Copy link
Contributor

resolve #17514

@k8s-ci-robot
Copy link

Hi @datbeohbbh. 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.

@datbeohbbh datbeohbbh force-pushed the fix/skip-when-detect-removed-peer branch from 05c294a to 46d59a2 Compare March 4, 2024 14:23
@ahrtr
Copy link
Member

ahrtr commented Mar 4, 2024

/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

@ahrtr ahrtr merged commit 8c3bb5a into etcd-io:main Mar 5, 2024
41 checks passed
@serathius
Copy link
Member

Can we add a test? Any test.

@ahrtr
Copy link
Member

ahrtr commented Mar 7, 2024

Can we add a test? Any test.

It should be a very safe & very minor change change. We don't have to necessarily add a test for this PR.

Also we already have some membership related e2e and integration test cases. I think it's OK.

@serathius
Copy link
Member

For me every bug that is worth backporting is also worth testing against.

@ahrtr
Copy link
Member

ahrtr commented Mar 9, 2024

I do not see how to reproduce this, so actually this PR fixed an non-exist issue. The fix just looks more natural (and of course safe and very minor) so we accepted it.

There is no point to waste time to get stuck on this. If you insist on adding a test, then provide detailed guide on how to do it. Or just close the backporting PRs or even revert the fix on main branch. I am not going to waste time on this, leave it to other maintainers & reviewer to take care of it since you have concern on it.

@serathius
Copy link
Member

Thanks for confirming that this is not an issue. No need to backport it then.

I didn't expect that writing a unit test needs a detailed guideline. Please let me know what would make writing unit test harder in etcd so we need guideline for this?

My assumption is that backporting can have a unexpected consequences due to code differences, and it's a good practice to have a test that confirms the backport was done correctly. Looked into contributor documentation I think we are missing a backport guidelines that would make it more clear. Let me file an issue for that.

@ahrtr
Copy link
Member

ahrtr commented Mar 12, 2024

I didn't expect that writing a unit test needs a detailed guideline. Please let me know what would make writing unit test harder in etcd so we need guideline for this?

Unit test doesn't always fit for each kind of fix. Understanding the root cause is the prerequisite, otherwise it doesn't make sense to request to add a test.

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.

When process messages to send to peers, leader should skip checking message type when detect removed peer.
5 participants