ndimiduk commented on code in PR #6722:
URL: https://github.com/apache/hbase/pull/6722#discussion_r1971493693
##########
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);
Review 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.
##########
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) {
Review Comment:
nit: you're already Streaming, so just keep streaming and return the result
of `anyMatch` or `findAny`.
I often find the transition between Stream and iteration jarring...
##########
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 {
+
+ public MetaTableIsolationConditional(BalancerConditionals
balancerConditionals,
+ BalancerClusterState cluster) {
+ super(balancerConditionals, cluster);
+ }
+
+ @Override
+ boolean isRegionToIsolate(RegionInfo regionInfo) {
+ return regionInfo.isMetaRegion();
Review 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.
##########
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) {
+ if (!getBalancerConditionals().isTableIsolationEnabled()) {
+ return BalanceAction.NULL_ACTION;
+ }
+
+ List<MoveRegionAction> moves = new ArrayList<>();
+ List<Integer> serverIndicesHoldingIsolatedRegions = new ArrayList<>();
+ int isolatedTableMaxReplicaCount = 1;
+ for (int serverIdx : cluster.getShuffledServerIndices()) {
+ if (EnvironmentEdgeManager.currentTime() > cluster.getStopRequestedAt())
{
+ break;
+ }
+ boolean hasRegionsToIsolate = false;
+ Set<Integer> regionsToMove = new HashSet<>();
+
+ // Move non-target regions away from target regions,
+ // and track replica counts so we know how many isolated hosts we need
+ for (int regionIdx : cluster.regionsPerServer[serverIdx]) {
+ RegionInfo regionInfo = cluster.regions[regionIdx];
+ if (shouldBeIsolated(regionInfo)) {
+ hasRegionsToIsolate = true;
+ int replicaCount = regionInfo.getReplicaId() + 1;
+ if (replicaCount > isolatedTableMaxReplicaCount) {
+ isolatedTableMaxReplicaCount = replicaCount;
+ }
+ } else {
+ regionsToMove.add(regionIdx);
+ }
+ }
+
+ if (hasRegionsToIsolate) {
+ serverIndicesHoldingIsolatedRegions.add(serverIdx);
+ }
+
+ // Generate non-system regions to move, if applicable
+ if (hasRegionsToIsolate && !regionsToMove.isEmpty()) {
+ for (int regionToMove : regionsToMove) {
+ for (int i = 0; i < cluster.numServers; i++) {
+ int targetServer = pickOtherRandomServer(cluster, serverIdx);
+ MoveRegionAction possibleMove =
+ new MoveRegionAction(regionToMove, serverIdx, targetServer);
+ if (!getBalancerConditionals().isViolating(cluster, possibleMove))
{
+ if (isWeighing) {
+ return possibleMove;
Review Comment:
Sorry, what does `weighing` mean ? Why early return here?
##########
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) {
+ if (!getBalancerConditionals().isTableIsolationEnabled()) {
+ return BalanceAction.NULL_ACTION;
+ }
+
+ List<MoveRegionAction> moves = new ArrayList<>();
+ List<Integer> serverIndicesHoldingIsolatedRegions = new ArrayList<>();
+ int isolatedTableMaxReplicaCount = 1;
+ for (int serverIdx : cluster.getShuffledServerIndices()) {
+ if (EnvironmentEdgeManager.currentTime() > cluster.getStopRequestedAt())
{
+ break;
+ }
+ boolean hasRegionsToIsolate = false;
+ Set<Integer> regionsToMove = new HashSet<>();
Review Comment:
We go to all this trouble to avoid collections, and yet here you ... this
tracking by index is so annoying.
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestLargeClusterBalancingTableIsolationAndReplicaDistribution.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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 static
org.apache.hadoop.hbase.master.balancer.CandidateGeneratorTestUtil.isTableIsolated;
+import static
org.apache.hadoop.hbase.master.balancer.CandidateGeneratorTestUtil.runBalancerToExhaustion;
+
+import java.util.*;
Review Comment:
I'm pretty sure that checkstyle will complain about splat imports.
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestLargeClusterBalancingTableIsolationAndReplicaDistribution.java:
##########
@@ -0,0 +1,118 @@
+/*
+ * 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 static
org.apache.hadoop.hbase.master.balancer.CandidateGeneratorTestUtil.isTableIsolated;
+import static
org.apache.hadoop.hbase.master.balancer.CandidateGeneratorTestUtil.runBalancerToExhaustion;
+
+import java.util.*;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.RegionInfoBuilder;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Category(MediumTests.class)
+public class TestLargeClusterBalancingTableIsolationAndReplicaDistribution {
+
+ @ClassRule
+ public static final HBaseClassTestRule CLASS_RULE = HBaseClassTestRule
+
.forClass(TestLargeClusterBalancingTableIsolationAndReplicaDistribution.class);
+
+ private static final Logger LOG =
+
LoggerFactory.getLogger(TestLargeClusterBalancingTableIsolationAndReplicaDistribution.class);
+
+ private static final TableName META_TABLE_NAME =
TableName.valueOf("hbase:meta");
Review Comment:
nit: we have `public static final TableName META_TABLE_NAME` in `TableName`.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestMetaTableIsolationBalancerConditional.java:
##########
@@ -0,0 +1,175 @@
+/*
+ * 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 static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNotEquals;
+
+import java.io.IOException;
+import java.util.Collection;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import java.util.stream.Collectors;
+import org.apache.hadoop.hbase.HBaseTestingUtil;
+import org.apache.hadoop.hbase.HConstants;
+import org.apache.hadoop.hbase.HRegionLocation;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.Admin;
+import org.apache.hadoop.hbase.client.ColumnFamilyDescriptorBuilder;
+import org.apache.hadoop.hbase.client.Connection;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.TableDescriptor;
+import org.apache.hadoop.hbase.client.TableDescriptorBuilder;
+import org.apache.hadoop.hbase.quotas.QuotaUtil;
+import org.apache.hadoop.hbase.testclassification.LargeTests;
+import org.apache.hadoop.hbase.util.Bytes;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import org.apache.hbase.thirdparty.com.google.common.collect.ImmutableSet;
+
+@Category(LargeTests.class)
+public class TestMetaTableIsolationBalancerConditional {
+
+ private static final Logger LOG =
+ LoggerFactory.getLogger(TestMetaTableIsolationBalancerConditional.class);
+ private static final HBaseTestingUtil TEST_UTIL = new HBaseTestingUtil();
+
+ private static final int NUM_SERVERS = 3;
+
+ @Before
+ public void setUp() throws Exception {
+
TEST_UTIL.getConfiguration().setBoolean(BalancerConditionals.ISOLATE_META_TABLE_KEY,
true);
+ TEST_UTIL.getConfiguration().setBoolean(QuotaUtil.QUOTA_CONF_KEY, true);
// for another table
+ TEST_UTIL.getConfiguration().setLong(HConstants.HBASE_BALANCER_PERIOD,
1000L);
+
TEST_UTIL.getConfiguration().setBoolean("hbase.master.balancer.stochastic.runMaxSteps",
true);
+
+ TEST_UTIL.startMiniCluster(NUM_SERVERS);
+ }
+
+ @After
+ public void tearDown() throws Exception {
+ TEST_UTIL.shutdownMiniCluster();
+ }
+
+ @Test
+ public void testTableIsolation() throws Exception {
+ Connection connection = TEST_UTIL.getConnection();
+ Admin admin = connection.getAdmin();
+
+ // Create "product" table with 3 regions
+ TableName productTableName = TableName.valueOf("product");
+ TableDescriptor productTableDescriptor =
TableDescriptorBuilder.newBuilder(productTableName)
+
.setColumnFamily(ColumnFamilyDescriptorBuilder.newBuilder(Bytes.toBytes("0")).build())
+ .build();
+ admin.createTable(productTableDescriptor,
+ BalancerConditionalsTestUtil.generateSplits(2 * NUM_SERVERS));
+
+ Set<TableName> tablesToBeSeparated = ImmutableSet.<TableName> builder()
+
.add(TableName.META_TABLE_NAME).add(QuotaUtil.QUOTA_TABLE_NAME).add(productTableName).build();
+
+ // Pause the balancer
+ admin.balancerSwitch(false, true);
+
+ // Move all regions (product, meta, and quotas) to one RegionServer
+ List<RegionInfo> allRegions = tablesToBeSeparated.stream().map(t -> {
+ try {
+ return admin.getRegions(t);
+ } catch (IOException e) {
+ throw new RuntimeException(e);
+ }
+ }).flatMap(Collection::stream).toList();
+ String targetServer =
+
TEST_UTIL.getHBaseCluster().getRegionServer(0).getServerName().getServerName();
+ for (RegionInfo region : allRegions) {
+ admin.move(region.getEncodedNameAsBytes(), Bytes.toBytes(targetServer));
+ }
+
+ validateRegionLocationsWithRetry(connection, tablesToBeSeparated,
productTableName, false,
+ false);
+
+ // Unpause the balancer and run it
+ admin.balancerSwitch(true, true);
+ admin.balance();
+
+ validateRegionLocationsWithRetry(connection, tablesToBeSeparated,
productTableName, true, true);
+ }
+
+ 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++) {
Review Comment:
nit: use a Waiter instead?
##########
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:
👍
##########
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
Review Comment:
nit: put the comment above or below ; don't split it over two lines.
##########
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) {
+ if (!getBalancerConditionals().isTableIsolationEnabled()) {
+ return BalanceAction.NULL_ACTION;
+ }
+
+ List<MoveRegionAction> moves = new ArrayList<>();
+ List<Integer> serverIndicesHoldingIsolatedRegions = new ArrayList<>();
+ int isolatedTableMaxReplicaCount = 1;
+ for (int serverIdx : cluster.getShuffledServerIndices()) {
+ if (EnvironmentEdgeManager.currentTime() > cluster.getStopRequestedAt())
{
Review Comment:
I'm surprised that there's not a simple `isStopped` or `isStopRequested`
boolean flag to pivot off of.
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/CandidateGeneratorTestUtil.java:
##########
@@ -233,6 +233,41 @@ static boolean
areAllReplicasDistributed(BalancerClusterState cluster) {
return true;
}
+ /**
+ * Generic method to validate table isolation.
+ */
+ static boolean isTableIsolated(BalancerClusterState cluster, TableName
tableName,
+ String tableType) {
+ for (int i = 0; i < cluster.numServers; i++) {
+ int[] regionsOnServer = cluster.regionsPerServer[i];
+ if (regionsOnServer == null || regionsOnServer.length == 0) {
+ continue; // Skip empty servers
+ }
+
+ boolean hasTargetTableRegion = false;
+ boolean hasOtherTableRegion = false;
+
+ for (int regionIndex : regionsOnServer) {
+ RegionInfo regionInfo = cluster.regions[regionIndex];
+ if (regionInfo.getTable().equals(tableName)) {
+ hasTargetTableRegion = true;
+ } else {
+ hasOtherTableRegion = true;
+ }
+
+ // If the target table and any other table are on the same server,
isolation is violated
+ if (hasTargetTableRegion && hasOtherTableRegion) {
+ LOG.warn(
Review Comment:
Both of these log messages seem more like DEBUG or TRACE to me. Why have you
set them as they are?
##########
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) {
+ if (!getBalancerConditionals().isTableIsolationEnabled()) {
+ return BalanceAction.NULL_ACTION;
+ }
+
+ List<MoveRegionAction> moves = new ArrayList<>();
+ List<Integer> serverIndicesHoldingIsolatedRegions = new ArrayList<>();
+ int isolatedTableMaxReplicaCount = 1;
+ for (int serverIdx : cluster.getShuffledServerIndices()) {
+ if (EnvironmentEdgeManager.currentTime() > cluster.getStopRequestedAt())
{
+ break;
Review Comment:
Should this return a NULL_ACTION instead of falling through to the rest of
the method?
... this method is long enough that I cannot easily tell the nesting from
cursory reading. Maybe the entirety of the remaining method body is inside this
for-loop?
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestLargeClusterBalancingMetaTableIsolation.java:
##########
@@ -0,0 +1,100 @@
+/*
+ * 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 static
org.apache.hadoop.hbase.master.balancer.CandidateGeneratorTestUtil.isTableIsolated;
+import static
org.apache.hadoop.hbase.master.balancer.CandidateGeneratorTestUtil.runBalancerToExhaustion;
+
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Set;
+import org.apache.hadoop.conf.Configuration;
+import org.apache.hadoop.hbase.HBaseClassTestRule;
+import org.apache.hadoop.hbase.ServerName;
+import org.apache.hadoop.hbase.TableName;
+import org.apache.hadoop.hbase.client.RegionInfo;
+import org.apache.hadoop.hbase.client.RegionInfoBuilder;
+import org.apache.hadoop.hbase.testclassification.MediumTests;
+import org.junit.BeforeClass;
+import org.junit.ClassRule;
+import org.junit.Test;
+import org.junit.experimental.categories.Category;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Category(MediumTests.class)
Review Comment:
Don't forget the `MasterTests` annotation on these tests.
Would it be handy to introduce a BalancerTests annotation?
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/SlopFixingCandidateGenerator.java:
##########
@@ -63,6 +64,13 @@ BalanceAction generateCandidate(BalancerClusterState
cluster, boolean isWeighing
List<MoveRegionAction> moves = new ArrayList<>();
Set<ServerAndLoad> fixedServers = new HashSet<>();
for (int sourceServer : sloppyServerIndices) {
+ if (
+ isTableIsolationEnabled
+ && getBalancerConditionals().isServerHostingIsolatedTables(cluster,
sourceServer)
+ ) {
+ // Don't fix sloppiness of servers hosting isolated tables
Review Comment:
Do we have a hook for excluding such servers from the sloppiness metric?
I.e., if the only slop is from this case, will the slop metrics continue to be
non-zero?
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/SlopFixingCandidateGenerator.java:
##########
@@ -89,7 +97,7 @@ BalanceAction generateCandidate(BalancerClusterState cluster,
boolean isWeighing
fixedServers.forEach(s -> cs.getServersByLoad().remove(s));
fixedServers.clear();
if (!regionFoundMove) {
- LOG.debug("Could not find a destination for region {} from server
{}.", regionIdx,
+ LOG.trace("Could not find a destination for region {} from server
{}.", regionIdx,
Review Comment:
Since its nested in double for-loops, probably better to wrap it in a
`if(isTraceEnabled()` branch. I have no micro-benchmark to support this
suggestion.
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestLargeClusterBalancingConditionalReplicaDistribution.java:
##########
@@ -107,7 +107,7 @@ public void testReplicaDistribution() {
runBalancerToExhaustion(conf, serverToRegions,
Set.of(CandidateGeneratorTestUtil::areAllReplicasDistributed), 10.0f);
- LOG.info("Meta table and system table regions are successfully isolated, "
+ LOG.info("Meta table regions are successfully isolated, "
Review Comment:
This method is testing replica distribution but the log talks about meta
table. The test doesn't look like it's working with meta replicas. What am I
missing?
--
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]
