[
https://issues.apache.org/jira/browse/SOLR-18298?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18112116#comment-18112116
]
Chris M. Hostetter commented on SOLR-18298:
-------------------------------------------
I'm not a Curator expert, but if the premise of the Jira is sound, then the
code changes in pr#4774 seem correct.
But the new tests seem too loose to be useful?
* ZkControllerTest
** Re-using the same ZkController for both of the "logical checks" (what
happens after "killServer()" and what happens after
"injectSessionExpiration()") means that the "A transient ZooKeeper reconnect
must not trigger session-expiration recovery" assertion might happen too early
relative when a currator background thread calls the reconnectListener
*** maybe it's currently true that _if_ reconnectListener is called, it is
called before findConnectionInstance(curatorClient.getZooKeeper()) will change
– but i don't know that for a fact, even if it is a true I don't know that it's
documented & guaranteed, and there's really no need to write a test that has to
assume it either way
*** in general it's hard to write a test that (safely) waits to try and prove
something *doesn't* happen in a background thread
** I would suggest that testing the "killServer()" situation should create a
ZkController, then kill the server, then wait for current != connected, *THEN*
close the ZkController, then and only then: check that at no point was the
"AtomicInteger recoveries" incremented.
** A completely separate ZkController should be used to test the
"injectSessionExpiration()" situation – but in this case there is shouldn't be
any reason for the artificially slow and CPU wasteful RetryUtils
*** instead the reconnectionHandler can release a ticket to an (initially
empty) semaphore, and the main test thread can call
"assertTrue(semaphore.tryAcquire(some reasonable limit))" immediately after
calling "injectSessionExpiration()"
* TestOnReconnectSessionExpiry
** testReconnectFiresForEveryReconnectedEvent - w/o assertions after every
stateChanged(), this test doesn't really prove anything about _which_
ConnectionStates result in the listener being fired
*** it could be the 3 RECONNECTs, or it could be the _first_ time any unique
ConnectionState is passed in ... either impl would pass this test
** again: having the assertions only at the end of the test doesn't tell us
anything about exactly what/when the callback is invoked depending on the
ConnectionState argument – as this test is written, either one of those
arguments could result in sessionExpired==true and the test would still pass
> ZkController.onReconnect Is Triggered Excessively
> -------------------------------------------------
>
> Key: SOLR-18298
> URL: https://issues.apache.org/jira/browse/SOLR-18298
> Project: Solr
> Issue Type: Bug
> Components: SolrCloud
> Affects Versions: 10.0
> Reporter: Jerry
> Priority: Blocker
> Labels: patch, pull-request-available, zookeeper
> Attachments:
> SOLR-18298_PR-4774_AtomicBoolean_Technical_Brief_Secure_Shrey_Narayan_Public_Citations.pdf,
> SOLR-18298_PR-4774_Technical_Review_NextBricks_Shrey Narayan.pdf
>
> Time Spent: 9h
> Remaining Estimate: 0h
>
> Curator's RECONNECTED event is different from the previous RECONNECTED event.
> Before Solr10, the OnReconnect is only triggered after a session expiration
> and reconnection.
> 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 is disconnected from a ZooKeeper instance and reconnected, 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.
> The PR restores the Solr 9 behavior, triggering re-election and other
> initialization staff only when the session actually expires, which
> significantly reduces performance overhead during network hiccups or
> situations like rolling-restart Zookeeper Cluster.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
