ctubbsii commented on PR #5192: URL: https://github.com/apache/accumulo/pull/5192#issuecomment-2557894083
I ended up addressing my last comment here, so I won't create a follow-on issue. Basically, the concern was that Watchers will not trigger if the session is dropped and we have to replace it with a new session (a new ZooKeeper client instance, since the session is tied to the instance). But Watchers will still see the session expired event. Poorly written watchers are at risk of not being fired ever again, if they don't monitor for session expired events, but well-written watchers should be fine. They just need to use a new ZooKeeper client object to re-set the Watcher if they want to start getting new event notifications. The old ZooSession kind of did that behind the scenes, but it was a little messy, because there was a lot of criss-crossing and tangled APIs. This PR changes that now to abstract ZooKeeper objects behind a new ZooSession object. This new object can be used in place of ZooKeeper, and holds a delegate ZooKeeper internally. When that one's session expires, it creates a replacement seamlessly internally. The methods are a subset of the methods on ZooKeeper. We can add more, as needed, and we can consider rolling in the APIs from ZooReader and ZooReaderWriter into this. A lot of other changes were included to remove the direct use of ZooKeeper objects everywhere in the code, and to replace them with this. The other major changes included here are a cleanup of ClientInfo/ServerInfo in the way they bootstrap ClientContext/ServerContext, so they can more easily reuse ZooKeeper objects (specifically, this new ZooSession object) efficiently, and all the different entry points into ServerInfo, in particular, to support the various use cases (normal server, testing, server-side utility with client properties config file, initialize etc.) are streamlined in the implementation and all lazily loaded via Guava Supplier memoization, with care taken to ensure that there are no cyclic dependencies on the instanceId/instanceName lookups and other things, in particular during Initialize where that's the greatest risk. The lazy loading also ensures that we're not wasting resources on establishing any connections to HDFS/ZK for AccumuloClient objects that are crea ted, but not used (in case the user creates a lot of them or something). Some follow-on work that could be done after this is fully tested and merged: 1. Consider putting ZooReader/ZooReaderWriter methods directly into the new ZooSession and getting rid of those 2. Consider catching session expired KeeperExceptions from the proxied methods, like ZooReader does, to not throw errors that are transient and recoverable by ZooSession 3. Also consider catching and dealing with InterruptedException from the ZooSession APIs, so that they don't bleed everywhere throughout Accumulo internal APIs (we'll still have to handle the ones from our own sleeps, but we shouldn't have to deal with the ones from ZooKeeper APIs outside of ZooSession) 4. Review all remaining Watcher objects for correct handling of session expiration -- 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]
