linxiaokun528 commented on PR #4577: URL: https://github.com/apache/solr/pull/4577#issuecomment-4941677391
> Regarding this thread @linxiaokun528 @ercsonusharma > > > FWIW, The public OnReconnect/OnDisconnect interfaces published in solrj-zookeeper, so removing them is a breaking change. The bug is purely in the detection logic which is fixable while keeping the interfaces intact. > > ... > > > While OnReconnect is triggered, we use AtomicBoolean.compareAndSet to set it back to false. But in this way, the two methods are tightly coupled. > > If you look at when `OnReconnect` is [triggered in 9.X](https://github.com/apache/solr/blob/releases/solr/9.10.1/solr/solrj-zookeeper/src/java/org/apache/solr/common/cloud/ConnectionManager.java#L156-L200) it is effectively an `OnExpired` hook. I think it always has been until the curator migration so your implementation of on "expired" is compatible with the original `OnReconnect`. The downside is that to not break the API you have to continue this misleading naming convention. One could easily argue this misleading naming convention is what caused this bug in the first place (confusion around what is a reconnection vs what is a session expiry + reconnection). So to echo @dsmiley's point I think from a purely _technical_ standpoint it is better to get rid of `OnReconnect`. Now whether to replace it with a `OnSessionExpiryReconnect` abstract class or use this new enum with anonymous class is another matter. I personally don't have strong feelings either way. > > Regarding `OnDisconnect` specifically this is a brand new addition in 10.X so IMO it is ok to drop since it has never really worked (I don't consider 10.0 to really be a working/stable release yet given problems like the one addressed by this PR). @kotman12 We can't use an abstract class. There are classes already `extends` something and need to implement the event. Previously, `OnReconnect` is an interface, which is fine. But we can't `extends` two classes. But the way, I don't like the previous interface `OnReconnect`, which used a default implementation as the following. It's kind of hacky. ``` default void stateChanged(CuratorFramework client, ConnectionState newState) { if (ConnectionState.RECONNECTED.equals(newState)) { onReconnect(); } } ``` -- 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]
