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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
