Uh oh!
There was an error while loading. Please reload this page.
HBASE-29074 Balancer conditionals should support meta table isolation - #6722
Conversation
d3d6f4d to
d587239Compare| } | ||
| if (LOG.isDebugEnabled()) { | ||
| if (LOG.isTraceEnabled()) { |
There was a problem hiding this comment.
These logs are really noisy in tests, and low value imo, so I've demoted them to trace
| boolean isServerHostingIsolatedTables(BalancerClusterState cluster, int serverIdx) { | ||
| Set<TableIsolationConditional> tableIsolationConditionals = | ||
| conditionals.stream().filter(TableIsolationConditional.class::isInstance) | ||
| .map(TableIsolationConditional.class::cast).collect(Collectors.toSet()); | ||
| for (TableIsolationConditional tableIsolationConditional : tableIsolationConditionals) { | ||
| if (tableIsolationConditional.isServerHostingIsolatedTables(cluster, serverIdx)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This helps us power sloppy server evaluation that's compatible with table isolation
| * If enabled, this class will help the balancer ensure that the meta table lives on its own | ||
| * RegionServer. Configure this via {@link BalancerConditionals#ISOLATE_META_TABLE_KEY} | ||
| */ | ||
| class MetaTableIsolationConditional extends TableIsolationConditional { |
There was a problem hiding this comment.
This implementation, and the MetaTableIsolationCandidateGenerator, are so lightweight because I anticipate adding a SystemTableIsolationConditional/CandidateGenerator soon
| BalanceAction batchMovesAndResetClusterState(BalancerClusterState cluster, | ||
| List<MoveRegionAction> moves) { | ||
| if (moves.isEmpty()) { | ||
| return BalanceAction.NULL_ACTION; | ||
| } |
There was a problem hiding this comment.
Returning a null action is better here, because it signifies that there's no work to be done by the generator — whereas a batch, even an empty one, is easily misinterpreted as actionable work
| if ( | ||
| !balancerConditionals.isTableIsolationEnabled() // table isolation is inherently incompatible | ||
| // with naive "sloppy server" checks | ||
| && sloppyRegionServerExist(cs) | ||
| ) { |
There was a problem hiding this comment.
In reality, I still think we will fix "sloppy" servers more effectively than a non-conditional balancer implementation by way of the SlopFixingCandidateGenerator (which is table isolation compatible, and performs better than the cost function based approach in my testing)
| return generateCandidate(cluster, false); | ||
| } | ||
| BalanceAction generateCandidate(BalancerClusterState cluster, boolean isWeighing) { |
There was a problem hiding this comment.
This PR is pretty straightforward in my opinion; if there's any head scratching complexity, then it's in this method. I'm happy to explore breaking up this method, which is admittedly probably too long, but doing so would probably introduce a much larger diff because we'd need more objects to represent the state that should be passed around across the goals of isolation + colocation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
ndimiduk
left a comment
There was a problem hiding this comment.
Mostly just comments and questions. Really nice to see how these play out in test.
| } | ||
| if (LOG.isDebugEnabled()) { | ||
| if (LOG.isTraceEnabled()) { |
Uh oh!
There was an error while loading. Please reload this page.
| boolean shouldSkipSloppyServerEvaluation() { | ||
| return isConditionalBalancingEnabled(); | ||
| boolean isTableIsolationEnabled() { | ||
| return conditionalClasses.contains(MetaTableIsolationConditional.class); |
There was a problem hiding this comment.
This strategy of feature enablement by class presence is interesting. I wonder how we'll have to change this approach if we introduce user-provided implementations in the future. Just a thought.
There was a problem hiding this comment.
When I introduce SystemTableIsolation I anticipate making this an isAssignableFrom check much like isReplicaDistributionEnabled
| @Override | ||
| boolean isRegionToIsolate(RegionInfo regionInfo) { | ||
| return regionInfo.isMetaRegion(); |
There was a problem hiding this comment.
Is it kind of a code smell that both the CandidateGenerator and the Conditional implementation have repeated logic? As you land your additional implementations, I wonder if this will suggest a change to the interfaces.
There was a problem hiding this comment.
probably yes, let me think about reusability maybe. But I might chalk this up as a relatively small imperfection
There was a problem hiding this comment.
I'm going to leave this alone for now, I think it's a disappointing but small reality of the logical divide between conditionals and candidate generators. Will keep thinking on this as I introduce the system table isolation conditional 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.
| private static void validateRegionLocationsWithRetry(Connection connection, | ||
| Set<TableName> tableNames, TableName productTableName, boolean areDistributed, | ||
| boolean runBalancerOnFailure) throws InterruptedException, IOException { | ||
| for (int i = 0; i < 100; i++) { |
There was a problem hiding this comment.
The same limitations apply that make me hesitant to use a waiter — I don't see how callbacks are supported to the degree that I need. Am I looking in the wrong place with Waiter#waitFor(...Predicate...)
This comment has been minimized.
This comment has been minimized.
797f85f to
0cd9bd1Compare
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.
| if (this == o) { | ||
| return true; | ||
| } | ||
| if (!(o instanceof ReplicaKey other)) { |
There was a problem hiding this comment.
This change adheres to our restrictions around new language features, please see #6729
This comment has been minimized.
This comment has been minimized.
0cd9bd1 to
c1314d7Compare
This comment has been minimized.
This comment has been minimized.
Apache-HBase
commented
Feb 27, 2025
🎊 +1 overall
This message was automatically generated. |
Apache-HBase
commented
Feb 27, 2025
🎊 +1 overall
This message was automatically generated. |
…#6722) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…#6722) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…#6722) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…#6722) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…#6722) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…apache#6722) (apache#6737) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…ta table isolation (apache#6722) (apache#6737) (will be in 2.7) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…ta table isolation (apache#6722) (apache#6737) (will be in 2.7) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…ta table isolation (apache#6722) (apache#6737) (will be in 2.7) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…ta table isolation (apache#6722) (apache#6737) (will be in 2.7) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
…apache#6722) (apache#6737) Signed-off-by: Nick Dimiduk <ndimiduk@apache.org> Co-authored-by: Ray Mattingly <rmattingly@hubspot.com>
In HBASE-28513 we added support for balancer conditionals which will the balancer discrete rules to follow when generating plans. This PR adds support for meta table isolation that works in a more flexible, lightweight, and cost effective way compared to RegionServer groups.
In a subsequent PR I'll solve HBASE-29075 by extending this framework to support generic system table isolation as well. The triad of 28513, 29074, and 29075 will complete my team's initial vision for balancer conditionals, unlocking support for system table isolation, meta table isolation, and improved secondary replica distribution.