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.

Sign up for GitHub

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

Jump to bottom

server: fix learner metric incorrect issue #17176

Merged
merged 1 commit into from
Jan 12, 2024
Merged

server: fix learner metric incorrect issue #17176

merged 1 commit into from
Jan 12, 2024

Conversation

YaoC
Copy link
Contributor

@YaoC YaoC commented Dec 29, 2023

@k8s-ci-robot
Copy link

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

@jmhbnz
Copy link
Member

jmhbnz commented Dec 29, 2023

/ok-to-test

fuweid
fuweid reviewed Dec 29, 2023
View reviewed changes
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

Thanks!

server/etcdserver/server.go Outdated Show resolved Hide resolved
@fuweid
Copy link
Member

fuweid commented Dec 29, 2023

I believe we should backport this into release/3.4 and release/3.5

@YaoC YaoC force-pushed the fix-learner-metric branch 2 times, most recently from aae07a5 to e5ef381 Compare December 29, 2023 07:57
@fuweid

This comment was marked as duplicate.

Sign in to view
@fuweid
Copy link
Member

fuweid commented Dec 29, 2023

Failure test case is related to #17054.

@fuweid
Copy link
Member

fuweid commented Dec 29, 2023

/retest

jmhbnz
jmhbnz approved these changes Dec 29, 2023
View reviewed changes
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 @YaoC

@serathius
Copy link
Member

Can you please add a test? e2e test would be the best. Please let me know if you need help with implementing it.

@YaoC YaoC force-pushed the fix-learner-metric branch 2 times, most recently from 2114f0b to 31340c5 Compare January 8, 2024 14:06
@YaoC
Copy link
Contributor Author

YaoC commented Jan 8, 2024

Can you please add a test? e2e test would be the best. Please let me know if you need help with implementing it.

Thanks @serathius, I've added the e2e tests, PTAL.

@YaoC
Copy link
Contributor Author

YaoC commented Jan 8, 2024

Failed test case may related #16327

@YaoC
Copy link
Contributor Author

YaoC commented Jan 8, 2024

/retest

@fuweid fuweid mentioned this pull request Jan 9, 2024
Closed
4 tasks
serathius
serathius approved these changes Jan 9, 2024
View reviewed changes
Copy link
Member

@serathius serathius left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for adding the tests. Fix is correct, however I would recommend refactoring the code to make the logic more consistent.

I would recommend to move handling of isLearner under the s.cluster field. The calls to updateLearnerMetric you added, should be part of s.cluster.Recover call. I would recommend to do this cleanup as a followup.

@YaoC
Copy link
Contributor Author

YaoC commented Jan 10, 2024 •
edited
Loading

I would recommend to move handling of isLearner under the s.cluster field. The calls to updateLearnerMetric you added, should be part of s.cluster.Recover call. I would recommend to do this cleanup as a followup.

You are right, perhaps I can do it in this PR, it looks better.

@ahrtr
Copy link
Member

ahrtr commented Jan 11, 2024

perhaps I can do it in this PR, it looks better.

Please feel free to update it in this PR if you want.

@YaoC
server: fix learner metric incorrect issue
f7ab7ad 
Signed-off-by: YaoC <chengyao09@hotmail.com>
@YaoC
Copy link
Contributor Author

YaoC commented Jan 12, 2024

I moved the metric isLearner to package membership, please help review it again. Thanks!

ahrtr
ahrtr approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

lgtm

Leave to @serathius to take a second look & merge

serathius
serathius approved these changes Jan 12, 2024
View reviewed changes
Copy link
Member

@serathius serathius left a comment

Choose a reason for hiding this comment

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

LGTM

Noticed that we are missing a test for member promotion.
Would appreciate if you could add it as a followup.

@serathius serathius merged commit 40f22e9 into etcd-io:main Jan 12, 2024
39 checks passed
@YaoC YaoC deleted the fix-learner-metric branch January 13, 2024 03:41
@fuweid
Copy link
Member

fuweid commented Jan 17, 2024

Hi @YaoC would you please file pull requests to 3.4 and 3.5 branches? Thanks

@YaoC
Copy link
Contributor Author

YaoC commented Jan 17, 2024

Hi @YaoC would you please file pull requests to 3.4 and 3.5 branches? Thanks

Sure, currently I'm working on these 😄

@YaoC YaoC mentioned this pull request Jan 17, 2024
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fuweid fuweid fuweid left review comments

@jmhbnz jmhbnz jmhbnz approved these changes

@ahrtr ahrtr ahrtr approved these changes

@serathius serathius serathius approved these changes

Assignees
No one assigned
Labels
backport/v3.4 backport/v3.5 ok-to-test
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Metric etcd_server_is_learner doesn't return the expected value after snapshot trigger
6 participants
@YaoC @k8s-ci-robot @jmhbnz @fuweid @serathius @ahrtr