NextbrickInc commented on code in PR #4774:
URL: https://github.com/apache/solr/pull/4774#discussion_r3867694505
##########
solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/OnReconnect.java:
##########
@@ -26,14 +29,36 @@
* implementation should call
* org.apache.solr.cloud.ZkController#removeOnReconnectListener(OnReconnect)
when it no longer needs
* to be notified of ZK reconnection events.
+ *
+ * <p>When registered as a Curator {@link ConnectionStateListener}, {@link
#onReconnect()} runs only
+ * after a session expiration ({@link ConnectionState#LOST} then {@link
+ * ConnectionState#RECONNECTED}). A reconnect after {@link
ConnectionState#SUSPENDED} keeps the
+ * ZooKeeper session and must not trigger SolrCloud recovery. See SOLR-18298.
*/
public interface OnReconnect extends ConnectionStateListener {
void onReconnect();
@Override
default void stateChanged(CuratorFramework client, ConnectionState newState)
{
- if (ConnectionState.RECONNECTED.equals(newState)) {
+ if (newState == ConnectionState.LOST) {
+ LostSessions.mark(this);
+ } else if (newState == ConnectionState.RECONNECTED &&
LostSessions.consume(this)) {
onReconnect();
}
}
+
+ /** Tracks listeners that have observed {@link ConnectionState#LOST} and
still need reconnect. */
+ final class LostSessions {
Review 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 21cb72bc75 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 21cb72bc75.
- 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 #4774.
Thanks.
[SOLR-18298_PR-4774_Technical_Review_NextBricks_Shrey
Narayan.pdf](https://github.com/user-attachments/files/31493049/SOLR-18298_PR-4774_Technical_Review_NextBricks_Shrey.Narayan.pdf)
Please approve, merge sir.
Please let me know if you need anything Else.
--
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]
