accumulo icon indicating copy to clipboard operation
accumulo copied to clipboard

Adding TabletServerLocks utility to accumulo admin command

Open cshannon opened this issue 2 years ago • 3 comments

Issue #2807

cshannon avatar Jul 16 '22 14:07 cshannon

I think this one should go under Admin command as well.

milleruntime avatar Jul 19 '22 13:07 milleruntime

@milleruntime - This has been updated to move to the Admin command

cshannon avatar Jul 22 '22 16:07 cshannon

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 like locks.

Sounds good, I'll make these changes

cshannon avatar Jul 31 '22 14:07 cshannon

@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.

cshannon avatar Aug 06 '22 11:08 cshannon

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.

Manno15 avatar Aug 06 '22 13:08 Manno15

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.

ctubbsii avatar Aug 08 '22 16:08 ctubbsii

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 avatar Aug 09 '22 12:08 milleruntime

@milleruntime - In your update you removed the main method, do you still want to do that now or do it later like the others?

cshannon avatar Aug 12 '22 12:08 cshannon

@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.

milleruntime avatar Aug 12 '22 13:08 milleruntime

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.

milleruntime avatar Aug 12 '22 14:08 milleruntime