Explore refactoring TabletLocator code #2301
Labels
enhancement
This issue describes a new feature, improvement, or optimization.
Comments
milleruntime
added
the
enhancement
|
Another factor that makes this change difficult is the use of |
|
@keith-turner - is this still an issue with your changes to the TabletLocator in 4.0? |
Its not clear to me what specific action was desired for this issue, so I am not sure. |
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
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
The
TabletLocatoris an abstract class with multiple concrete classes, all maintaining static state locations of tablets for clients. These classes also cache information about mutations and ranges for reading and writing to a table. TheTabletLocatorabstract class itself is registered as a singleton with the SingletonManager. This is probably the most complicated singleton registered as you have to deal with the implementation in each child class as well. It seems likeTabletLocatorcould exist inClientContextas most of the abstract methods take it as a parameter.Here are the different types of
TabletLocator:The text was updated successfully, but these errors were encountered: