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

[improve][meta] Stop election operations when LeaderElectionImpl gets closed #23995

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

lhotari
Copy link
Member

@lhotari lhotari commented Feb 17, 2025

Motivation

In some tests, I noticed that LeaderElectionImpl could continue to make operations and override the closed state.

Modifications

Add checks if the internalState is Closed

Documentation

  • doc
  • doc-required
  • doc-not-needed
  • doc-complete

@lhotari lhotari added this to the 4.1.0 milestone Feb 17, 2025
@lhotari lhotari self-assigned this Feb 17, 2025
@github-actions github-actions bot added the doc-not-needed Your PR changes do not impact docs label Feb 17, 2025
@lhotari lhotari force-pushed the lh-fix-LeaderElectionImpl-close branch from 2d4c72e to a62e10a Compare February 18, 2025 09:24
@@ -113,6 +116,11 @@ private synchronized CompletableFuture<LeaderElectionState> elect() {
return tryToBecomeLeader();
}
}).thenCompose(leaderElectionState -> {
synchronized (this) {
if (internalState == InternalState.Closed) {
Copy link
Member

Choose a reason for hiding this comment

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

Need to add volatile on internalState?

Copy link
Member Author

Choose a reason for hiding this comment

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

Already covered with synchronized (this). this points to LeaderElectionImpl instance here.

@lhotari lhotari requested a review from dao-jun February 18, 2025 13:57
@lhotari lhotari marked this pull request as draft February 18, 2025 13:58
@lhotari
Copy link
Member Author

lhotari commented Feb 18, 2025

Fixing a potentially flaky test that uses a bad way to trigger leader election.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants