Explore refactoring ZooCacheFactory #2299
Labels
enhancement
This issue describes a new feature, improvement, or optimization.
Comments
milleruntime
added
the
enhancement
|
@EdColeman is working on versioned properties in ZK to reduce the number of watchers per table, among other benefits (I don't have the ticket numbers immediately available to reference). That work is replacing the use of ZooCache with another caching mechanism for config properties stored in ZK. Since config properties are a substantial use case for ZooCache, it might sense to limit the scope of any refactoring here, in order to avoid wasting effort refactoring for uses that are being dropped and to focus on the uses that will remain. |
ctubbsii
added a commit
to ctubbsii/accumulo
that referenced
this issue
Dec 23, 2024
* Replace existing ZooSession with a new one that is a facade for ZooKeeper. The methods from ZooKeeper we need are placed on it and internally it maintains a delegate ZooKeeper instance that handles automatically creating a new ZooKeeper session (client object) when the old session has died. This enables us to maintain fewer ZooKeeper objects by keeping only one at a time per ClientContext/ServerContext, and to reduce complexity substantially, where it was hard to reason about which ZooKeeper session we were using at any given moment. This no longer requires passing around ZooKeeper objects via ZooReader that called to the old ZooSession to construct a ZooKeeper session on demand. The new ZooSession is now a singleton field whose lifecycle is part of the context, and ZooReader/ZooReaderWriter are substantially simplified. * Lazily construct objects in ServerInfo/ClientInfoImpl to simplify the implementation for the various use cases, and to ensure that we don't create more objects than needed. * Improve debugging information to tie the ZooSession instances with their purpose (for example, for use with ServerContext, or for use with ClientContext, with the user's name) * Get rid of ZooCacheFactory in favor of a lazily constructed ZooCache instance in the ClientContext, to make it more clear when a ZooCache is being shared or reused, and to remove a static singleton * Move instanceId and instanceName lookup logic into ZooUtil * Make ZookeeperLockChecker use its own ZooSession, because it is still a static singleton and must continue to operate after the context that constructed it is closed (should be fixed when apache#2301 is done) * Perform some minor improvements to ZooCache to simplify its constructors now that it uses ZooSession, and to change the type of external watchers to Consumer, so they aren't as easily confused with actual ZooKeeper watchers * Improve a lot of ZooKeeper related test code Potential future work after this could include: * Roll ZooReader and ZooReaderWriter functions directly into ZooSession * Add support to ZooSession for more ZooKeeper APIs * Handle KeeperException thrown from delegate that signals the session is disconnected, instead of relying only on the verifyConnected() method in ZooSession to update the delegate * Handle InterruptedExceptions directly in ZooSession, so they don't propagate everywhere in the code in ways that are inconvenient to handle This fixes apache#5124, apache#2299, and apache#2298
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This class is registered as a singleton with SingletonManager. It might be possible to drop this class by moving the getZooCache() methods to their respective places in ServerContext and ClientContext.
Some of the server code uses this factory to get the systemConfig in
ZooConfigurationFactoryandServerConfigurationFactory, both of which store static objects. TheServerInfoobject uses this class to get the cache in a few different ways. The different types ofAccumuloConfigurationobjects also use this class to get the property cache.The old and new client code uses this class to get and store the ZooCache. The static helper class
Tablesuses ZooCacheFactory internally to get ZooCache on demand across different static methods such as exists(), getTableMap() and getTableState().The text was updated successfully, but these errors were encountered: