accumulo
accumulo copied to clipboard
Adding TabletServerLocks utility to accumulo admin command
Issue #2807
I think this one should go under Admin command as well.
@milleruntime - This has been updated to move to the Admin command
Looks good so far but I think the command could be improved a little bit. The default behavior (if the user doesn't provide a parameter) should just be to list the locks. And the command
tServerLocks
is a bit awkward to type. It could be shortened to something likelocks
.
Sounds good, I'll make these changes
@milleruntime - Can this issue and also #2814 be merged or do we want to try and get rid of SiteConfiguration now? I think it makes more sense to merge as is and get rid of SiteConfiguration as a follow on task since more refactoring and work would probably be needed.
Can this issue and also https://github.com/apache/accumulo/pull/2814 be merged or do we want to try and get rid of SiteConfiguration now? I think it makes more sense to merge as is and get rid of SiteConfiguration as a follow on task since more refactoring and work would probably be needed.
@milleruntime I am personally in favor of merging this as is and discussing whether or not we want to go through and remove siteConfiguration from each of these commands that use them.
Without taking a deeper look, I'm not sure what this utility is specifically doing, so I can't speak to the benefits of where its entry point should be. However, one comment I'll make about SiteConfiguration is: the point of removing SiteConfiguration was to ensure that we don't have a dependency, or in general, make any assumptions about where the shell was being run, relative to where server processes are deployed. For example, there's no reason to assume that the server configuration file (the site configuration), with the instance.secret
for writing to protected nodes in ZooKeeper, is available on the class path for the shell. The shell is a client application, and should not be assumed to have access to the server's configuration file at all.
The same is not true for utilities specifically written to assist system administrators perform maintenance on Accumulo itself on the server side. Many such utilities are required to be run on the server itself, so it is reasonable for them to assume they have access to the server configuration on their class path.
Let's be careful not to lose site of the point of getting SiteConfiguration out of the shell, and forcing all these utilities to run client side, and then needing to bake in convoluted hacks to get them to run off the server. It is perfectly acceptable for server-side utilities to just assume access to server-side configuration and class path.
Thanks @ctubbsii for keeping things in perspective. I think this server utility fits well under the Admin
command but I think the code could be cleaned up. I am going to take a look at it.
@milleruntime - In your update you removed the main method, do you still want to do that now or do it later like the others?
@milleruntime - In your update you removed the main method, do you still want to do that now or do it later like the others?
You can just ignore that update to your branch for now.
OK @cshannon I think I got all your PRs that had additions to the Admin command. Please just have 1 follow up PR with all the improvements.