Conversation
Curator's RECONNECTED event is different from the previous RECONNECTED event. Before Solr10, the OnReconnect is only triggered after a reconnection from a session expiration. Check the following https://github.com/apache/solr/blob/fdb5314279657f7895a90123436d834e81ea3157/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ConnectionManager.java#L165 https://github.com/apache/solr/blob/fdb5314279657f7895a90123436d834e81ea3157/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ConnectionManager.java#L199 But Curator's RECONNECTED event is triggered every time a Solr node is disconnected from a ZooKeeper instance and reconnected to another ZooKeeper instance. Therefore, currently ZkController.onReconnect is invoked every time a Solr node reconnects to a ZooKeeper instance without a session expiration, which is a huge overhead, especially when we need to rolling-restart a ZooKeeper Cluster. It can take more than 10 minutes for a small Solr cluster to level out. Similiarly, now ZkController.onDisconnect is triggered just after a disconnection from a Zookeeper instance. It should only be triggered after a session expiration.
| final String leaderPath; | ||
|
|
||
| /** Parent of {@link #leaderPath}; derived once since {@link #leaderPath} is final. */ | ||
| final String leaderParentPath; |
There was a problem hiding this comment.
Both overseer and shard leader share this concept so I wanted to centralize it.
| // (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) { |
There was a problem hiding this 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
| } catch (Exception e) { | ||
| log.warn("Unable to rejoinElection ", e); | ||
| if (zkController != null && !zkController.getCoreContainer().isShutDown()) { | ||
| zkController.rejoinOverseerElection(null, false); |
There was a problem hiding this 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.
| if (!this.isClosed && !overseer.getZkController().getCoreContainer().isShutDown()) { | ||
| boolean shutDown = overseer.getZkController().getCoreContainer().isShutDown(); | ||
| if (!this.isClosed && !shutDown) { | ||
| registerLeaderNode(Utils.toJSON(myProps)); |
There was a problem hiding this 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".
| synchronized (this) { | ||
| if (leaderZkNodeParentVersion != null) { | ||
| try { | ||
| deleteLeaderNode(); |
There was a problem hiding this 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.
| if (log.isInfoEnabled()) { | ||
| log.info("Quit command received {} {}", message, LeaderElector.getNodeName(myId)); | ||
| } | ||
| quitReceived = true; |
There was a problem hiding this 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.
| * election fails with NodeExists. | ||
| */ | ||
| @Test | ||
| public void testOverseerWedgesOnExpiryRacingReconnect() throws Exception { |
There was a problem hiding this comment.
Btw this test reproduces with more realistic / less contrived configurations (15-30s session timeout and 2s tick window) but in the interest of build times I tuned those down.
| } catch (Exception e) { | ||
| log.warn("Unable to rejoinElection ", e); | ||
| if (zkController != null && !zkController.getCoreContainer().isShutDown()) { | ||
| zkController.rejoinOverseerElection(null, false); |
There was a problem hiding this comment.
We can probably remove the zkController != null check as the Overseer is now only initialized from within a zkController. Looks like zkController could be marked final as well.
|
@HoustonPutman @markrmiller I'd greatly appreciate it if either of you could take a look. |
| 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) { |
There was a problem hiding this 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.
2 participants
AI Use
I used Claude Opus 5 to greatly refine the test which is sensitive to timing. The model also aided in code research of the existing logic. The process was very slow and took many tens of iterations and supplementary "manual" checking of work. I take full responsibility of the output.
Description
Reproducing the stuck-overseer bug caused by close vs start data race. Previously overseer would enter leader election only on session expiry. By triggering election on every reconnect we uncovered certain bugs that temporarily became a lot more common with the unintended reconnect behavior. Since that issue was patched I can still recreate this issue though it is more sensitive to timing.
After digging even deeper it seems that curator behavior itself may be driving the residual bug as well, although in a more subtle way. Curator tears down the overseer with its own, internally managed timer. This can happen without the server-side session ephemerals being torn down (as I show in the tests) since ZK's check to destroy these happens on a tick-wide interval and is not synchronized at all with what Curator does (Curator's own docs acknowledge this). This difference in behavior also contributes to this bug. The net effect is you can have a permanently locked Overseer election that won't budge unless you manually restart.
Solution
Although returning to the previous reconnect behavior was a good start (see #4577) it is still possible to hit this when you have a really ill-timed reconnect that happens within a zk tick of a session boundary causing two election threads to interfere with one another. The solution is to synchronize the critical section where we write the existing overseer leader node and actually start the overseer. Before the leader node was written outside of the critical section and could survive the closure of its underlying overseer (blocking the subsequent one from taking over).
A further enhancement made here is to only launch the
OverseerExitThreadwhen a non-ZK event is triggering the tear-down of the Overseer. This is because our zk-registered callbacks already manage the Overseer lifecycle. Having a competing Overseer "lineage" (from the OET) only adds noise.A final enhancement is to add optimistic version checks when reading/writing the Overseer leader node to prevent ABA-style interference across nodes (centralizing the logic because the shard leader election already does this).
Tests
Checklist
Please review the following and check all that apply:
mainbranch../gradlew check.