kotman12 commented on code in PR #4625:
URL: https://github.com/apache/solr/pull/4625#discussion_r4065008476
##########
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) {
Review Comment:
It is weird that we don't synchronize `super.cancelElection` or
`overseer.close` within this block. However, the current logic already permits
unsynchronized cancel vs overseer.close and the bug in question is not related
to this. Theoretically, it means that two threads can be tearing down the same
overseer and OverseerElectionContext (taking turns in the various sections).
It's also worth pointing out that the lifecycle of the overseer (singleton)
is quite different from that of the "enclosing" `OverseerElectionContext`
(roughly one per generation). As far as I can tell they are not robustly
synchronized and the ownership structure is not well-defined. In general this
model could be greatly improved outside of this change which I tried to keep as
minimal as I could to address a practical problem.
--
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]
