You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by "kotman12 (via GitHub)" <gi...@apache.org> on 2026/09/18 20:15:33 UTC
Re: [PR] SOLR-18301 overseer election does not converge [solr]
kotman12 commented on code in PR #4625:
URL: https://github.com/apache/solr/pull/4625#discussion_r4050247655
##########
solr/core/src/java/org/apache/solr/cloud/ElectionContext.java:
##########
@@ -31,9 +36,21 @@ public abstract class ElectionContext implements Closeable {
final ZkNodeProps leaderProps;
final String id;
final String leaderPath;
+
+ /** Parent of {@link #leaderPath}; derived once since {@link #leaderPath} is final. */
+ final String leaderParentPath;
Review Comment:
Both overseer and shard leader share this concept so I wanted to centralize it.
##########
solr/core/src/java/org/apache/solr/cloud/Overseer.java:
##########
@@ -453,44 +467,15 @@ private void checkIfIamStillLeader() {
&& (zkController.getCoreContainer().isShutDown() || zkController.isClosed())) {
return; // shutting down no need to go further
}
- Stat stat = new Stat();
- final String path = OVERSEER_ELECT + "/leader";
- byte[] data;
- try {
- data = zkClient.getData(path, null, stat);
- } catch (IllegalStateException | KeeperException.NoNodeException e) {
- return;
- } catch (Exception e) {
- log.warn("Error communicating with ZooKeeper", e);
- return;
- }
+ // We only reach here after a QUIT (roles handoff) or an unexpected crash, i.e. cases where no
+ // Zk reconnect handler will re-drive the election. The rejoin below cancels our context,
+ // which is what removes our leader registration.
try {
- Map<?, ?> m = (Map<?, ?>) Utils.fromJSON(data);
- String id = (String) m.get(ID);
- if (overseerCollectionConfigSetProcessor.getId().equals(id)) {
- try {
- log.warn(
- "I (id={}) am exiting, but I'm still the leader",
- overseerCollectionConfigSetProcessor.getId());
- zkClient.delete(path, stat.getVersion());
- } catch (KeeperException.BadVersionException e) {
- // no problem ignore it some other Overseer has already taken over
- } catch (Exception e) {
- log.error("Could not delete my leader node {}", path, e);
- }
-
- } else {
- log.info("somebody else (id={}) has already taken up the overseer position", id);
- }
- } finally {
- // if I am not shutting down, Then I need to rejoin election
- try {
- if (zkController != null && !zkController.getCoreContainer().isShutDown()) {
- zkController.rejoinOverseerElection(null, false);
- }
- } catch (Exception e) {
- log.warn("Unable to rejoinElection ", e);
+ if (zkController != null && !zkController.getCoreContainer().isShutDown()) {
+ zkController.rejoinOverseerElection(null, false);
Review Comment:
The `rejoinOverseerElection` here is guaranteed to first cancel the current "election" and by doing so it will `close` this `Overseer`. This, in turn, will delete the leader node.
##########
solr/core/src/java/org/apache/solr/cloud/OverseerElectionContext.java:
##########
@@ -61,18 +60,41 @@ void runLeaderProcess(boolean weAreReplacement) throws KeeperException, Interrup
final String id = leaderSeqPath.substring(leaderSeqPath.lastIndexOf('/') + 1);
ZkNodeProps myProps = new ZkNodeProps(ID, id);
- zkClient.makePath(leaderPath, Utils.toJSON(myProps), CreateMode.EPHEMERAL);
-
+ // Register and start under the same lock close() takes, so a close() cannot land between them
+ // and leave a leader znode with no overseer behind it. Registration also captures the parent
+ // version so cancelElection() only deletes our own. Mirrors ShardLeaderElectionContextBase.
synchronized (this) {
- if (!this.isClosed && !overseer.getZkController().getCoreContainer().isShutDown()) {
+ boolean shutDown = overseer.getZkController().getCoreContainer().isShutDown();
+ if (!this.isClosed && !shutDown) {
+ registerLeaderNode(Utils.toJSON(myProps));
Review Comment:
Registering the leader node _within_ the synchronized block is the critical change of this patch. Without it there is a possibility of "election interference".
##########
solr/core/src/java/org/apache/solr/cloud/OverseerElectionContext.java:
##########
@@ -61,18 +60,41 @@ void runLeaderProcess(boolean weAreReplacement) throws KeeperException, Interrup
final String id = leaderSeqPath.substring(leaderSeqPath.lastIndexOf('/') + 1);
ZkNodeProps myProps = new ZkNodeProps(ID, id);
- zkClient.makePath(leaderPath, Utils.toJSON(myProps), CreateMode.EPHEMERAL);
-
+ // Register and start under the same lock close() takes, so a close() cannot land between them
+ // and leave a leader znode with no overseer behind it. Registration also captures the parent
+ // version so cancelElection() only deletes our own. Mirrors ShardLeaderElectionContextBase.
synchronized (this) {
- if (!this.isClosed && !overseer.getZkController().getCoreContainer().isShutDown()) {
+ boolean shutDown = overseer.getZkController().getCoreContainer().isShutDown();
+ if (!this.isClosed && !shutDown) {
+ registerLeaderNode(Utils.toJSON(myProps));
+ log.info("Created overseer leader registration {} -> {}", leaderPath, id);
overseer.start(id);
+ } else {
+ log.info(
+ "Not registering as overseer leader for {}: isClosed={}, shutDown={}",
+ leaderPath,
+ this.isClosed,
+ shutDown);
}
}
}
@Override
public void cancelElection() throws InterruptedException, KeeperException {
super.cancelElection();
+ // Delete only our own registration, guarded by the parent version captured at registration, so
+ // we can never remove a newer lineage's (ABA-safe). Mirrors ShardLeaderElectionContextBase.
+ synchronized (this) {
+ if (leaderZkNodeParentVersion != null) {
+ try {
+ deleteLeaderNode();
Review Comment:
I moved this here because that is how the shard leader context manages this and it seems reasonable. I don't see why we wouldn't want to be consistent here.
##########
solr/core/src/java/org/apache/solr/cloud/Overseer.java:
##########
@@ -570,6 +555,7 @@ private List<ZkWriteCommand> processMessage(
if (log.isInfoEnabled()) {
log.info("Quit command received {} {}", message, LeaderElector.getNodeName(myId));
}
+ quitReceived = true;
Review Comment:
For posterity, this appears to be an "internal API" which lets the Cloud implement overseer node prioritization, i.e. designating/preferring some nodes to be Overseer over others.
##########
solr/core/src/java/org/apache/solr/cloud/Overseer.java:
##########
@@ -389,14 +391,26 @@ public void run() {
refreshClusterState = true; // it might have been a bad version error
}
}
+ } catch (Throwable t) {
+ // The main loop terminated abnormally -- not a clean close, not a session-expiry return,
+ // not a QUIT. Rejoin below so we recover instead of leaving a dead overseer still holding
+ // the /overseer_elect/leader znode with nothing behind it.
+ crashed = true;
+ log.error("Overseer main loop terminated unexpectedly", t);
} finally {
if (log.isInfoEnabled()) {
log.info("Overseer Loop exiting : {}", LeaderElector.getNodeName(myId));
}
- // do this in a separate thread because any wait is interrupted in this main thread
- Thread checkLeaderThread = new Thread(this::checkIfIamStillLeader, "OverseerExitThread");
- checkLeaderThread.setDaemon(true);
- checkLeaderThread.start();
+ // Only spawn the exit thread to rejoin the election when nobody else will: an explicit QUIT
+ // (roles handoff) or an unexpected crash. On a clean close or a ZK session-expiry
+ // reconnect, the ZkController reconnect handler owns re-election, so spawning here would
+ // just race it and risk two competing overseer lineages.
+ if (quitReceived || crashed) {
Review Comment:
The reason we disambiguate these from the case where ZK/curator callback closes the overseer (due to disconnect or whatever) is that when zk callback does the closing then the zk/curator machinery presumably owns the election rejoin as well. So when we run `OverseerExitThread` in that case it actually needlessly races the zk-callback triggered rejoin. With the concurrency bug fixes in this PR this shouldn't be a critical issue however it adds unnecessary noise and churn. It also doesn't appear to be the original intention behind `OverseerExitThread`
##########
solr/core/src/java/org/apache/solr/cloud/Overseer.java:
##########
@@ -453,44 +467,15 @@ private void checkIfIamStillLeader() {
&& (zkController.getCoreContainer().isShutDown() || zkController.isClosed())) {
return; // shutting down no need to go further
}
- Stat stat = new Stat();
- final String path = OVERSEER_ELECT + "/leader";
- byte[] data;
- try {
- data = zkClient.getData(path, null, stat);
- } catch (IllegalStateException | KeeperException.NoNodeException e) {
- return;
- } catch (Exception e) {
- log.warn("Error communicating with ZooKeeper", e);
- return;
- }
+ // We only reach here after a QUIT (roles handoff) or an unexpected crash, i.e. cases where no
+ // Zk reconnect handler will re-drive the election. The rejoin below cancels our context,
+ // which is what removes our leader registration.
try {
- Map<?, ?> m = (Map<?, ?>) Utils.fromJSON(data);
- String id = (String) m.get(ID);
- if (overseerCollectionConfigSetProcessor.getId().equals(id)) {
- try {
- log.warn(
- "I (id={}) am exiting, but I'm still the leader",
- overseerCollectionConfigSetProcessor.getId());
- zkClient.delete(path, stat.getVersion());
- } catch (KeeperException.BadVersionException e) {
- // no problem ignore it some other Overseer has already taken over
- } catch (Exception e) {
- log.error("Could not delete my leader node {}", path, e);
- }
-
- } else {
- log.info("somebody else (id={}) has already taken up the overseer position", id);
- }
- } finally {
- // if I am not shutting down, Then I need to rejoin election
- try {
- if (zkController != null && !zkController.getCoreContainer().isShutDown()) {
- zkController.rejoinOverseerElection(null, false);
- }
- } catch (Exception e) {
- log.warn("Unable to rejoinElection ", e);
+ if (zkController != null && !zkController.getCoreContainer().isShutDown()) {
+ zkController.rejoinOverseerElection(null, false);
Review Comment:
We can probably delete this `zkController != null` check as the `Overseer` is only initialized from within the `ZkController`
--
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: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org