rmdmattingly commented on code in PR #6722:
URL: https://github.com/apache/hbase/pull/6722#discussion_r1969979205
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerClusterState.java:
##########
@@ -313,16 +313,16 @@ protected BalancerClusterState(Map<ServerName,
List<RegionInfo>> clusterState,
regionIndex++;
}
- if (LOG.isDebugEnabled()) {
+ if (LOG.isTraceEnabled()) {
Review Comment:
These logs are really noisy in tests, and low value imo, so I've demoted
them to trace
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java:
##########
@@ -434,7 +434,11 @@ boolean needsBalance(TableName tableName,
BalancerClusterState cluster) {
return true;
}
- if (sloppyRegionServerExist(cs)) {
+ if (
+ !balancerConditionals.isTableIsolationEnabled() // table isolation is
inherently incompatible
+ // with naive "sloppy
server" checks
+ && sloppyRegionServerExist(cs)
+ ) {
Review 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)
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerConditionals.java:
##########
@@ -90,8 +94,20 @@ boolean isReplicaDistributionEnabled() {
.anyMatch(DistributeReplicasConditional.class::isAssignableFrom);
}
- boolean shouldSkipSloppyServerEvaluation() {
- return isConditionalBalancingEnabled();
+ boolean isTableIsolationEnabled() {
+ return conditionalClasses.contains(MetaTableIsolationConditional.class);
+ }
+
+ 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;
}
Review Comment:
This helps us power sloppy server evaluation that's compatible with table
isolation
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/RegionPlanConditionalCandidateGenerator.java:
##########
@@ -64,8 +64,11 @@ BalanceAction generate(BalancerClusterState cluster) {
return balanceAction;
}
- MoveBatchAction batchMovesAndResetClusterState(BalancerClusterState cluster,
+ BalanceAction batchMovesAndResetClusterState(BalancerClusterState cluster,
List<MoveRegionAction> moves) {
+ if (moves.isEmpty()) {
+ return BalanceAction.NULL_ACTION;
+ }
Review 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
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/MetaTableIsolationConditional.java:
##########
@@ -0,0 +1,37 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.balancer;
+
+import org.apache.hadoop.hbase.client.RegionInfo;
+
+/**
+ * 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 {
Review Comment:
This implementation, and the MetaTableIsolationCandidateGenerator, are so
lightweight because I anticipate adding a
SystemTableIsolationConditional/CandidateGenerator soon
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/TableIsolationCandidateGenerator.java:
##########
@@ -0,0 +1,131 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hbase.master.balancer;
+
+import java.util.ArrayList;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.util.EnvironmentEdgeManager;
+import org.apache.yetus.audience.InterfaceAudience;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
[email protected]
+public abstract class TableIsolationCandidateGenerator
+ extends RegionPlanConditionalCandidateGenerator {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(TableIsolationCandidateGenerator.class);
+
+ TableIsolationCandidateGenerator(BalancerConditionals balancerConditionals) {
+ super(balancerConditionals);
+ }
+
+ abstract boolean shouldBeIsolated(RegionInfo regionInfo);
+
+ @Override
+ BalanceAction generate(BalancerClusterState cluster) {
+ return generateCandidate(cluster, false);
+ }
+
+ BalanceAction generateCandidate(BalancerClusterState cluster, boolean
isWeighing) {
Review 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 is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
