Uh oh!
There was an error while loading. Please reload this page.
HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves - #3536
Conversation
9a6544b to
6520805CompareApache-HBase
commented
Jul 28, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 28, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 28, 2021
💔 -1 overall
This message was automatically generated. |
6520805 to
eca1e48CompareApache-HBase
commented
Jul 28, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 28, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Jul 28, 2021
🎊 +1 overall
This message was automatically generated. |
joshelser
left a comment
There was a problem hiding this comment.
Nice idea. I think we could take this good idea and make it a great idea though :)
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bbeaudreault
commented
Jul 30, 2021
@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
787c350Comparebbeaudreault
commented
Aug 5, 2021
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). |
Apache-HBase
commented
Aug 5, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2021
💔 -1 overall
This message was automatically generated. |
8f4808a to
850b042Compareec09f26 to
ee1dec2CompareApache-HBase
commented
Aug 6, 2021
🎊 +1 overall
This message was automatically generated. |
ee1dec2 to
8bedb53Compare@joshelser@clarax This is ready for another look. To summarize the changes:
|
Apache-HBase
commented
Aug 6, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2021
Apache-HBase
commented
Aug 6, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 6, 2021
💔 -1 overall
This message was automatically generated. |
42d3dff to
f89b5b6Compare…nning the balancer without executing any region moves
c049fe3 to
4edd432CompareApache-HBase
commented
Aug 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 23, 2021
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
commented
Aug 23, 2021
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. |
Apache-HBase
commented
Aug 23, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 23, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
💔 -1 overall
This message was automatically generated. |
7995e64 to
e20b5f7CompareApache-HBase
commented
Aug 24, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
💔 -1 overall
This message was automatically generated. |
e20b5f7 to
279d251CompareApache-HBase
commented
Aug 24, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 24, 2021
🎊 +1 overall
This message was automatically generated. |
bbeaudreault
commented
Aug 24, 2021
@joshelser@ndimiduk fyi this is ready for review again. Tests are now passing. |
joshelser
left a comment
There was a problem hiding this comment.
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.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
bbeaudreault
commented
Aug 25, 2021
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. |
bbeaudreault
commented
Aug 27, 2021
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! :) |
Apache-HBase
commented
Aug 27, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 27, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 27, 2021
🎊 +1 overall
This message was automatically generated. |
joshelser
commented
Sep 2, 2021
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 theforceboolean. Adding a separatedryRunBalance()method seemed like the cleanest and most intuitive way to add this new functionality in a backwards compatible way.