-
Notifications
You must be signed in to change notification settings - Fork 3.4k
HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves #3536
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
9a6544b
to
6520805
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
6520805
to
eca1e48
Compare
🎊 +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.
Nice idea. I think we could take this good idea and make it a great idea though :)
hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java
Outdated
Show resolved
Hide resolved
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterDryRunBalancer.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterDryRunBalancer.java
Outdated
Show resolved
Hide resolved
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterDryRunBalancer.java
Outdated
Show resolved
Hide resolved
@joshelser I pushed a commit which addresses most of your feedback. Thanks again! I have not addressed the interface changes, pending your thoughts on my recent comments. Happy to address those and squash all the commits once we come to a decision. |
eca1e48
to
787c350
Compare
I just pushed a commit which makes the changes we talked about in the Admin and RSGroupAdmin interfaces. I still need to update the shell, will do that tomorrow. Overall I like how this came out, as I said I used a factory for now because we really don't have enough arguments to justify a builder. One thing I don't love is the duplication between hmaster and rsgroup, but that seems to be the name of the game with rsgroup for now. It caused a little bit of funkiness in order to handle the more strict RSGroupAdminServer interface though (i.e. the private BalanceRequest.forRunMode method). |
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
8f4808a
to
850b042
Compare
ec09f26
to
ee1dec2
Compare
🎊 +1 overall
This message was automatically generated. |
ee1dec2
to
8bedb53
Compare
@joshelser @clarax This is ready for another look. To summarize the changes:
|
🎊 +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. |
42d3dff
to
f89b5b6
Compare
…nning the balancer without executing any region moves
c049fe3
to
4edd432
Compare
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Hey all, here's an update:
I'm not able to run the full test suite right now, so will be keeping an eye on the jenkins build and update if any tests fail. I didn't add an explicit testBalancerResponses test, but I did include assertions about the new fields in some existing tests. |
🎊 +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. |
7995e64
to
e20b5f7
Compare
🎊 +1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
e20b5f7
to
279d251
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. |
@joshelser @ndimiduk fyi this is ready for review again. Tests are now passing. |
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.
A couple of suggestions around javadoc on public classes. Given the magnitude of this patch (and my tardiness on reviews), I'm OK to defer that to a follow-on.
The documentation and new tests included in this already is excellent. Wonderful work.
I like how the BalancerRequest/Response worked out on the API. I almost left a comment about re-using BalancerRequest on the BalanceRSGroupRequest (rather than duplicating the attributes), but decided against it.
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalanceRequest.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalanceRequest.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalanceResponse.java
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalanceResponse.java
Outdated
Show resolved
Hide resolved
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalanceResponse.java
Show resolved
Hide resolved
Thanks for the reviews everyone. I discussed with @joshelser and will submit a separate PR for documentation improvements since this one already has a long and storied past. If someone could merge, that'd be great. Unfortunately it does not apply cleanly to master, so I will work on a PR for that now. |
Soo, in the master port PR Duo asked that I remove the InterfaceStability annotations. I didn't realize those were not supposed to be on IA.Public. I just pushed a commit here to do that as well, and as part of that I addressed @joshelser's feedback about docs. Hopefully we're good now! :) |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
🎊 +1 overall
This message was automatically generated. |
Flakiness on TestFSHLogProvider. Passed on rerun. |
See https://issues.apache.org/jira/browse/HBASE-26147
I considered adding another overload of the existing
balance()
methods, but I felt there already existed a lot of overloads there between the deprecatedbalancer()
methods and theforce
boolean. Adding a separatedryRunBalance()
method seemed like the cleanest and most intuitive way to add this new functionality in a backwards compatible way.