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

: