-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-22903 : Table to RegionStatesCount metrics - Use for broken alter_status command #611
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
|
Please review @saintstack |
|
💔 -1 overall
This message was automatically generated. |
|
Please review @anoopsjohn @Reidddddd |
|
Confused, is the JIRA id correct? |
|
Yes, Jira Id is correct, however, the solution is generic i.e. use new metrics, which can be used for broken alter_status command. |
|
Good to know, i'm reviewing another big PR. |
2d103df to
bfa607f
Compare
|
💔 -1 overall
This message was automatically generated. |
|
@Apache9 could you please review this PR? |
| .addAllServersName(metrics.getServersName().stream().map(ProtobufUtil::toServerName) | ||
| .collect(Collectors.toList())); | ||
| .collect(Collectors.toList())) | ||
| .addAllTableRegionStatesCount(metrics.getTableRegionStatesCount() |
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 guess we can do a code format here ?
| status = @admin.getAlterStatus(org.apache.hadoop.hbase.TableName.valueOf(table_name)) | ||
| if status.getSecond != 0 | ||
| puts "#{status.getSecond - status.getFirst}/#{status.getSecond} regions updated." | ||
| cluster_metrics = @admin.getClusterMetrics |
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 think we need a jruby UT to address the alter_status ? Please also provide a Java UT to address the getTableRegionStatesCount interface, Thanks.
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.
Done
| table_region_status = cluster_metrics.getTableRegionStatesCount | ||
| .get(org.apache.hadoop.hbase.TableName.valueOf(table_name)) | ||
| if table_region_status.getTotalRegions != 0 | ||
| updated_regions = table_region_status.getTotalRegions - |
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.
The closed / opening region won't be RIT state ? if so , then we should minus the closed region & opening region etc.
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 believe we can minus closed region(Done), but opening should be part of RIT already
|
BTW, please also take a look the HBaseQA, seems checkstyles & ruby has some problem now, please update your PR if it's related. @virajjasani |
|
💔 -1 overall
This message was automatically generated. |
|
Thanks for the review, updated the PR based on comments, please take a look @openinx |
|
💔 -1 overall
This message was automatically generated. |
openinx
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.
Looks good to me overall, few nits left.
| return closedRegions; | ||
| } | ||
|
|
||
| private void setClosedRegions(int closedRegions) { |
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: let's try to align the get & set placement ? all getXX first, then setXX ? or getXX & setXX one by one ?
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.
Done @openinx
Thanks
openinx
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.
Looks good to me
|
💔 -1 overall
This message was automatically generated. |
…er_status command (#611) Signed-off-by: huzheng <[email protected]>
…er_status command (#611) Signed-off-by: huzheng <[email protected]>
…er_status command (#611) Signed-off-by: huzheng <[email protected]>
…er_status command (apache#611) Signed-off-by: huzheng <[email protected]>
…er_status command (apache#611) Signed-off-by: huzheng <[email protected]> (cherry picked from commit 4a37572) Change-Id: I7aa7bb1646bf1bfc2b8d9bd22c62877a54b0b956
Jira: https://issues.apache.org/jira/browse/HBASE-22903