-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-29323 Use Priority Handler for all RegionServerStatus rpc at Master(#6994) #6998
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
HBASE-29323 Use Priority Handler for all RegionServerStatus rpc at Master(#6994) #6998
Conversation
…ster (apache#6994) Co-authored-by: ukumawat <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nihal Jain <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Reviewed-by: Aman Poonia <[email protected]>
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
| splitWalProcedureDoneReport); | ||
| checkMethod(conf, "GetLastFlushedSequenceId", HConstants.HIGH_QOS, qosFunction, | ||
| lastFlushedSequenceIdRequest); | ||
| checkMethod(conf, "RegionServerReport", HConstants.HIGH_QOS, qosFunction, |
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.
also add corresponding test for checkMethod(conf, "CompactRegion", HConstants.ADMIN_QOS, qosFunction);?
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.
won't it make difference in branch-3 and branch-2 ?
While writing I didn't find a good way to add Test for all so just added for some important 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.
I guess the point is previous line 101 has checkMethod(conf, "CompactRegion", HConstants.ADMIN_QOS, qosFunction); and that check would still be valid in branch-2, so you don't need to remove that line here.
Ultimately, I'm not sure how important it is, though, since the method is removed in master branch as part of #3612 (d26bcaa and https://issues.apache.org/jira/browse/HBASE-25288)... and it's probably unnecessarily ADMIN_QOS here in 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.
added this as well.
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 why you didn't just leave it inside the testAnnotations method :)
|
LGTM, will merge tomorrow morning IST and pull back upto branch-2.6! Thanks @Umeshkumar9414 ! |
| splitWalProcedureDoneReport); | ||
| checkMethod(conf, "GetLastFlushedSequenceId", HConstants.HIGH_QOS, qosFunction, | ||
| lastFlushedSequenceIdRequest); | ||
| checkMethod(conf, "RegionServerReport", HConstants.HIGH_QOS, qosFunction, |
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 the point is previous line 101 has checkMethod(conf, "CompactRegion", HConstants.ADMIN_QOS, qosFunction); and that check would still be valid in branch-2, so you don't need to remove that line here.
Ultimately, I'm not sure how important it is, though, since the method is removed in master branch as part of #3612 (d26bcaa and https://issues.apache.org/jira/browse/HBASE-25288)... and it's probably unnecessarily ADMIN_QOS here in branch-2.
|
💔 -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. |
|
These failures caused by " Job manager has been shut down." @NihalJain last run was successful, after that I added that sigle test and that was running fine on local. Can you rerun the build and help me with this PR? |
I think we are fine as your last commit should not cause the failure. Let me merge this! |
…ster(#6994) (#6998) Co-authored-by: ukumawat <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nihal Jain <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Reviewed-by: Aman Poonia <[email protected]> (cherry picked from commit 19440ac)
…ster(apache#6994) (apache#6998) Co-authored-by: ukumawat <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nihal Jain <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Reviewed-by: Aman Poonia <[email protected]> (cherry picked from commit d187378)
…ster(apache#6994) (apache#6998) Co-authored-by: ukumawat <[email protected]> Signed-off-by: Duo Zhang <[email protected]> Signed-off-by: Nihal Jain <[email protected]> Signed-off-by: Viraj Jasani <[email protected]> Reviewed-by: Aman Poonia <[email protected]> (cherry picked from commit d187378)
HBASE-29323 Use Priority Handler for all RegionServerStatus rpc at Master(#6994)