SOLR-18298: only recover after ZooKeeper session expiry - #4774
SOLR-18298: only recover after ZooKeeper session expiry#4774NextbrickInc wants to merge 2 commits into
Conversation
…session expiry. Curator RECONNECTED fires on every ZK instance hop, which made rolling ZK restarts re-elect leaders and re-register cores. Restore Solr 9 behavior by treating LOST then RECONNECTED as expiration, authored by Shrey Narayan (NextBrick). Co-authored-by: Cursor <cursoragent@cursor.com>
|
@HoustonPutman @risdenk This implements the small-impact, detection-only approach requested in the #4577 discussion and keeps the existing callback interfaces. Validation against current main passes the focused 4-test suite, |
|
@HoustonPutman @risdenk Please approve, merge sir. |
There was a problem hiding this comment.
I don't think this change is needed.
| } | ||
|
|
||
| /** Tracks listeners that have observed {@link ConnectionState#LOST} and still need reconnect. */ | ||
| final class LostSessions { |
There was a problem hiding this comment.
I would really rather have an atomic boolean in ZkController and ZkStateReader that maintains this. It will eventually go away.
There was a problem hiding this comment.
Hi @HoustonPutman Thank you so much for your comments.
I have replied to both of them. Tested them. And fixed. Please review.
Pls review code change, pdf, comment below.
Please approve PR, merge code. Thank you so much.
Please let me know if you need anything Else.
Thanks, Houston — I reworked this along the ownership boundary you suggested. I reverted the semantic changes in OnDisconnect and OnReconnect, so those shared adapters retain their existing behavior. ZkController and standalone ZkStateReader now each maintain a private AtomicBoolean that is set only when OnDisconnect reports sessionExpired=true (Curator LOST). Their reconnect handlers use compareAndSet(true, false), so a transient SUSPENDED → RECONNECTED does not run full recovery, while LOST → RECONNECTED runs it once and atomically consumes the flag.
I added coverage for the unchanged listener contracts and a three-node ZooKeeper integration test that distinguishes ordinary ensemble failover from injected session expiration. ./gradlew tidy, the two focused test targets (2 listener tests + 1 integration test), and git diff --check pass. I also launched the patched build locally as SolrCloud on 8983 with embedded ZooKeeper on 9983 and verified commit 21cb72b in the Admin UI. This keeps the state local and temporary, so it can be removed cleanly when persistent watchers eliminate this recovery path.
@HoustonPutman — addressed both comments in 21cb72b.
- ZkController and ZkStateReader now own independent AtomicBooleans.
- Shared reconnect/disconnect callbacks retain their general behavior.
- LOST arms the flag; SUSPENDED does not.
- The first later RECONNECTED consumes the flag and runs recovery once.
Local verification passed: focused tests, ./gradlew tidy, and
./gradlew check -x test. The working tree is clean.
If this matches your requested design, please approve and merge PR SOLR-18298: only recover after ZooKeeper session expiry #4774. Thanks.
SOLR-18298_PR-4774_Technical_Review_NextBricks_Shrey Narayan.pdf
Please approve, merge sir.
Please let me know if you need anything Else.
There was a problem hiding this comment.
3 participants
Summary
Relationship to #4577
This is the smaller, detection-only implementation requested in that review discussion. It avoids the larger callback/event refactor while preserving the same session-expiry semantics.
Validation