-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-26730 Extend hbase shell 'status' command to support an option 'tasks' #4094
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
7b187aa to
cfd925e
Compare
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
Unit test failures are not related. Looks like it is about time to do a unit test cleanup on branch-2. |
gjacoby126
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.
Just a couple of nits
| serverMetricsMap = new HashMap<>(); | ||
| } | ||
| final Map<ServerName, ServerMetrics> map = serverMetricsMap; | ||
| serverManager.getOnlineServers().entrySet() |
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.
nit: could we factor out the common code into a helper rather than duplicating?
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 suppose it is possible to collapse this into a single code block, would that be better? E.g.
boolean doTasks = false;
....
case TASKS:
doTasks = true;
// fall through
case LIVE_SERVERS: {
...
// do stuff
if (doTasks) {
// do tasks stuff too, like the master monitoredtask collection
...
}
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 did take up your suggestion to D.R.Y this and was able to remove some lines, it looks good @gjacoby126
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Outdated
Show resolved
Hide resolved
cfd925e to
86ee7b4
Compare
|
Rebased and addresses @gjacoby126 's feedback |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
|
🎊 +1 overall
This message was automatically generated. |
gjacoby126
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.
+1, thanks @apurtell
|
As with the master PR will take care of the checkstyle issues before merge. |
…'tasks' Expose monitored tasks state in ClusterStatus API via new option in ServerLoad. Add shell support for interrogating monitored tasks state in ServerLoad.
…sks in 'tasks' view
86ee7b4 to
716dad9
Compare
…'tasks' (#4094) Signed-off-by: Geoffrey Jacoby <[email protected]> Signed-off-by: Viraj Jasani <[email protected]>
Expose monitored tasks state in ClusterStatus API via new option in ServerLoad.
Add shell support for interrogating monitored tasks state in ServerLoad.
Here's a prototype running on a branch-2 fork:

: