linxiaokun528 commented on PR #4577: URL: https://github.com/apache/solr/pull/4577#issuecomment-4882292574
> Thanks for tracking this down. It's fine to keep the current direction if you want the clean removal, but flagging that the same fix is achievable with much less surface change IMO. > > `SolrCuratorEvent` is more machinery than needed. It's an enum + abstract of() + a new EventAction type that's really just Runnable renamed. The same behavior can live directly on the existing OnReconnect/OnDisconnect callbacks and no new type. The AtomicBoolean that makes it correct just lives in the returned closure. > > 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. @ercsonusharma Thanks for reviewing. I created the new type EventAction/SolrCuratorEvent mainly for future usage. Curator does not contain events like expiredReconnect. Since we need different events from those provided by Curator, I think we should create our own event types. > The same behavior can live directly on the existing OnReconnect/OnDisconnect callbacks and no new type. The only way I can figure out this is to have an AtomicBoolean variable. When OnDisconnect is triggered, the variable is set to `true`. While OnReconnect is triggered, we use `AtomicBoolean.compareAndSet` to set it back to `false`. But in this way, the two methods are tightly coupled. Besides, as I mentioned above, we may need an event like expiredReconnect again in the future (or maybe now. We may have other places suffering the same kind of issue.) I think we should have an easier way to deal with this kind of event, instead of using AtomicBoolean every time. -- 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]
