You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by "xyuanlu (via GitHub)" <gi...@apache.org> on 2025/08/07 18:40:09 UTC
[PR] Fix a race condition in metaclient leader election [helix]
xyuanlu opened a new pull request, #3058:
URL: https://github.com/apache/helix/pull/3058
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
(#200 - Link your issue number here: You can write "Fixes #XXX". Please use the proper keyword so that the issue gets closed automatically. See https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue
Any of the following keywords can be used: close, closes, closed, fix, fixes, fixed, resolve, resolves, resolved)
### Description
- [X] Here are some details about my PR, including screenshots of any UI changes:
Fix a race condition in metaclient leader election
(Write a concise description including what, why, how)
### Tests
- [ ] The following tests are written for this issue:
(List the names of added unit/integration tests)
- The following is the result of the "mvn test" command on the appropriate module:
(If CI test fails due to known issue, please specify the issue and test PR locally. Then copy & paste the result of "mvn test" to here.)
### Changes that Break Backward Compatibility (Optional)
- My PR contains changes that break backward compatibility or previous assumptions for certain methods or API. They include:
(Consider including all behavior changes for public methods or API. Also include these changes in merge description so that other developers are aware of these changes. This allows them to make relevant code changes in feature branches accounting for the new method/API behavior.)
### Documentation (Optional)
- In case of new functionality, my PR adds documentation in the following wiki page:
(Link the GitHub wiki you added)
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu merged PR #3058:
URL: https://github.com/apache/helix/pull/3058
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "LZD-PratyushBhatt (via GitHub)" <gi...@apache.org>.
LZD-PratyushBhatt commented on code in PR #3058:
URL: https://github.com/apache/helix/pull/3058#discussion_r2267266821
##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/leaderelection/LeaderElectionClient.java:
##########
@@ -189,9 +195,11 @@ private void createPathIfNotExists(String path) {
}
private void subscribeAndTryCreateLeaderEntry(String leaderPath) {
- _metaClient.subscribeDataChange(leaderPath + LEADER_ENTRY_KEY, _reElectListener, false);
+ _leaderGroups.add(leaderPath + LEADER_ENTRY_KEY);
+ registerAllListeners();
Review Comment:
I have a question here.
Here's the comment as a paragraph:
what happens if listener re-subscription succeeds but participant node creation fails? What i mean is, currently, the flow would re-subscribe all listeners successfully, then if it fail to create the participant node (due to network issues, parent path missing, whatever), but still proceed to attempt leader node creation and touch the leader node. could this result in a participant becoming leader without being a valid member of the participant pool, leading to an invalid leader state that other participants don't recognize as legitimate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on PR #3058:
URL: https://github.com/apache/helix/pull/3058#issuecomment-3176829139
This change is ready to be merged. Approved by @junkaixue
Commit message "Fix a race condition in metaclient leader election."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #3058:
URL: https://github.com/apache/helix/pull/3058#discussion_r2261835812
##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/leaderelection/LeaderElectionClient.java:
##########
@@ -421,6 +430,23 @@ public void handleConnectStateChanged(MetaClientInterface.ConnectState prevState
LOG.info("Participant {} already in leader group {}.", _participant, leaderPath);
}
}
+ // resubscribe the re-elect listener
+ for (String leaderPath : _leaderGroups) {
+ LOG.info("Resubscribe re-elect listener for leaderPath {}.", leaderPath);
+ _metaClient.subscribeDataChange(leaderPath + LEADER_ENTRY_KEY, _reElectListener, false);
+ }
+
+ // resubscribe to leader entry change since we are reconnected
+ for (Map.Entry<String, Set<LeaderElectionListenerInterfaceAdapter>> entry: _leaderChangeListeners.entrySet()) {
+ LOG.info("Resubscribe leader change listener for leaderPath {}.", entry.getKey());
+ String leaderPath = entry.getKey();
+ Set<LeaderElectionListenerInterfaceAdapter> listeners = entry.getValue();
+ for (LeaderElectionListenerInterfaceAdapter listener : listeners) {
+ _metaClient.subscribeDataChange(leaderPath + LEADER_ENTRY_KEY,
+ listener, false /*skipWatchingNonExistNode*/); // we need to subscribe event when path is not there
+ _metaClient.subscribeStateChanges(listener);
+ }
+ }
// touch leader node to renew session ID
Review Comment:
why not have a function for this and we can apply to:
1. First time start
2. reconnect.
I think they are on different code path.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "LZD-PratyushBhatt (via GitHub)" <gi...@apache.org>.
LZD-PratyushBhatt commented on code in PR #3058:
URL: https://github.com/apache/helix/pull/3058#discussion_r2267266821
##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/leaderelection/LeaderElectionClient.java:
##########
@@ -189,9 +195,11 @@ private void createPathIfNotExists(String path) {
}
private void subscribeAndTryCreateLeaderEntry(String leaderPath) {
- _metaClient.subscribeDataChange(leaderPath + LEADER_ENTRY_KEY, _reElectListener, false);
+ _leaderGroups.add(leaderPath + LEADER_ENTRY_KEY);
+ registerAllListeners();
Review Comment:
Hey @xyuanlu thanks for this!
I have a question here.
what happens if listener re-subscription succeeds but participant node creation fails? What i mean is, currently, the flow would re-subscribe all listeners successfully, then if it fail to create the participant node (due to network issues, parent path missing, whatever), but still proceed to attempt leader node creation and touch the leader node. could this result in a participant becoming leader without being a valid member of the participant pool, leading to an invalid leader state that other participants don't recognize as legitimate?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "junkaixue (via GitHub)" <gi...@apache.org>.
junkaixue commented on code in PR #3058:
URL: https://github.com/apache/helix/pull/3058#discussion_r2261567646
##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/leaderelection/LeaderElectionClient.java:
##########
@@ -79,6 +82,8 @@ public class LeaderElectionClient implements AutoCloseable {
private final static String PARTICIPANTS_ENTRY_PARENT = "/PARTICIPANTS/";
ReElectListener _reElectListener = new ReElectListener();
ConnectStateListener _connectStateListener = new ConnectStateListener();
+ private final ConcurrentHashMap<String, Set<LeaderElectionListenerInterfaceAdapter>> _leaderChangeListener =
Review Comment:
Listeners.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "LZD-PratyushBhatt (via GitHub)" <gi...@apache.org>.
LZD-PratyushBhatt commented on code in PR #3058:
URL: https://github.com/apache/helix/pull/3058#discussion_r2268501464
##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/leaderelection/LeaderElectionClient.java:
##########
@@ -189,9 +195,11 @@ private void createPathIfNotExists(String path) {
}
private void subscribeAndTryCreateLeaderEntry(String leaderPath) {
- _metaClient.subscribeDataChange(leaderPath + LEADER_ENTRY_KEY, _reElectListener, false);
+ _leaderGroups.add(leaderPath + LEADER_ENTRY_KEY);
+ registerAllListeners();
Review Comment:
That makes sense, Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
Re: [PR] Fix a race condition in metaclient leader election [helix]
Posted by "xyuanlu (via GitHub)" <gi...@apache.org>.
xyuanlu commented on code in PR #3058:
URL: https://github.com/apache/helix/pull/3058#discussion_r2267340872
##########
meta-client/src/main/java/org/apache/helix/metaclient/recipes/leaderelection/LeaderElectionClient.java:
##########
@@ -189,9 +195,11 @@ private void createPathIfNotExists(String path) {
}
private void subscribeAndTryCreateLeaderEntry(String leaderPath) {
- _metaClient.subscribeDataChange(leaderPath + LEADER_ENTRY_KEY, _reElectListener, false);
+ _leaderGroups.add(leaderPath + LEADER_ENTRY_KEY);
+ registerAllListeners();
Review Comment:
In handleConnectStateChanged, where we recreate participant node, re-subscribe listeners and touch leader node. I think if recreate participant node failed in line 425 and if it is not nodeAlreadyExist error, it would through exception out and exit before register listeners and touch leader node.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org