Uh oh!
There was an error while loading. Please reload this page.
HBASE-26147: Add a dry run mode to the balancer, where moves are calculated but not actually executed - #3630
Closed
bbeaudreault wants to merge 2 commits into
Closed
Conversation
bbeaudreaultforce-pushed
the
dry_run_balancer_master
branch
from
August 26, 2021 13:04
b00398b to
df64523CompareApache-HBase
commented
Aug 26, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 26, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 26, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 26, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
commented
Aug 26, 2021
Contributor
The HBaseTestingUtility has been renamed to HBaseTestingUtil on master, which cause the compilation error. Anyway, I think this means we should try to push out 3.0.0 ASAP, as there are more and more incompatible changes between master and branch-2. |
bbeaudreault
commented
Aug 26, 2021
ContributorAuthor
Thank you Duo. I didn't notice this because my maven threw a NPE locally rather than any useful compile error (this jvm bug), so I was hoping the build server would help me. I ended up figuring it out and fixing the HBaseTestingUtil conflict by setting I agree it'd be nice to get moving on 3.0.0, it was pretty painful to port this particular patch. |
Apache-HBase
commented
Aug 26, 2021
💔 -1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 26, 2021
💔 -1 overall
This message was automatically generated. |
…ulated but not actually executed
bbeaudreaultforce-pushed
the
dry_run_balancer_master
branch
from
August 26, 2021 18:00
df64523 to
4000c72CompareApache-HBase
commented
Aug 26, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 26, 2021
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Aug 26, 2021
🎊 +1 overall
This message was automatically generated. |
Apache9
reviewed
Aug 27, 2021
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| * Encapsulates options for executing a run of the Balancer. | ||
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving |
Contributor
There was a problem hiding this comment.
There is no IS.Evolving for IA.Public classes, it should always be IS.Stable implicitly, so we do not add IS annotation for IA.Public classes.
ContributorAuthor
There was a problem hiding this comment.
Sorry, I misunderstood. I will remove. I have to say that it's a little unfortunate that there's no way with the current compatibility matrix to mark something as effectively beta. That's what I thought Evolving was for, and part of the reason we decided to break this out of the more stable Admin interface.
Member
There was a problem hiding this comment.
@Apache9 I'm also surprised by your interpretation of the interaction between IA.Public and IS.Evolving. The javadoc on InterfaceStability says
All classes that are annotated with {@link Public} or {@link LimitedPrivate} must have InterfaceStability annotation.
Indeed, InterfaceStability itself is annotated with IA.Public and IS.Evolving.
| * Builder for constructing a {@link BalanceRequest} | ||
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving |
| * Response returned from a balancer invocation | ||
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving |
| * Builds a {@link BalanceResponse} for returning results of a balance invocation to callers | ||
| */ | ||
| @InterfaceAudience.Public | ||
| @InterfaceStability.Evolving |
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. |
Apache9
approved these changes
Aug 28, 2021
joshelser
approved these changes
Aug 30, 2021
joshelser
left a comment
Member
There was a problem hiding this comment.
LGTM. Sorry I forgot that Public+Evolving was a no-go (you're right that we should have something in-between -- LimitedPrivate+Evolving might be the way to go but it's hard when we're talking about Admin and other long-standing API).
Only one final thought is that BalanceResponse$Builder probably shouldn't be Public. Use of the object is certainly Public, but how that object is created does not need to be public.
I think I'd rather see this land rather than another round of reviews. The last precommit round looks good to me (just some flakiness in tests). Let me try to do that tonight.
| /** | ||
| * Builds a {@link BalanceResponse} for returning results of a balance invocation to callers | ||
| */ | ||
| @InterfaceAudience.Public |
Member
There was a problem hiding this comment.
Thinking about it, we probably don't want the Builder class here to be Public. Users shouldn't ever be building this BalanceResponse on their own, just using the BalanceResponse.
ContributorAuthor
There was a problem hiding this comment.
I created https://issues.apache.org/jira/browse/HBASE-26240 for this. I'll submit a small PR once these existing PRs are merged.
ndimiduk
commented
Aug 30, 2021
Member
I left a comment on one of Duo's line comments, but I'll repeat it here. I'm also surprised by this interpretation of the interaction between
Indeed, @joshelser@Apache9 do we document the HBase project's interpretation of these annotations somewhere in our developer guide? |
joshelser
commented
Sep 1, 2021
Member
Yeah, we have this: https://hbase.apache.org/book.html#hbase.client.api.surface I think Bryan definitely found a gap in that we don't have a good way to represent user-facing but not yet stable. |
joshelser
commented
Sep 2, 2021
Member
Best as I can tell, the test failures were flakes which passed on re-run. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is the master port for #3536
I also addressed documentation feedback from #3536. This port was not super straightforward because of rsgroup being moved into
hbase-server. The move was not a simply file move, but code moved within various classes. Additionally, HBaseAdmin.java was removed in master in favor of AdminOverAsyncAdmin (and some other minor admin changes).I've read everything over, but it may be worth giving a quick look for correctness