-
Notifications
You must be signed in to change notification settings - Fork 478
Decompose admin command into separate commands #6118
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
There are two tests in CheckServerIT that are not passing |
|
Ran this locally to see what it looked like. This is great, could really improve and organize what is currently there. Was wondering about the following groupings of commands, not suggesting that for this PR just posting thoughts about eventual direction. Not sure if this would be good to do in this PR or not. Also, there is probably a lot of room for improvements in follow on issues to get more consistency across commands. Also wondering about the core, if there is a way to have those commands at the top level |
|
@keith-turner - no issues here with renaming and regrouping things, but I'd rather do it in follow-on PRs, just so that this doesn't continue to grow. |
ctubbsii
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure "admin" is the best catch-all because users can just throw new commands on the classpath. But, overall, I think this is headed in a good direction.
| * under the License. | ||
| */ | ||
| package org.apache.accumulo.server.util; | ||
| package org.apache.accumulo.server.util.adminCommand; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not a fan of mixed case in package names. How about this instead?
| package org.apache.accumulo.server.util.adminCommand; | |
| package org.apache.accumulo.server.util.admin.cli; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just following the established convention. ...util.checkCommand, ...util.fateCommand, and ...util.serviceStatus already exist. If we want to change the package names, I think we should do them all together in a different PR.
Closes #6086