ndimiduk commented on a change in pull request #3536: URL: https://github.com/apache/hbase/pull/3536#discussion_r685591340
########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/BalanceRequest.java ########## @@ -0,0 +1,96 @@ +/* + * + * 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; Review comment: Can this class be in any package that's more specific? `o.a.h.h.client`, for example? `client.admin` would be even better, but that looks like a larger project than this change. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java ########## @@ -1577,12 +1577,20 @@ public static IsMasterRunningRequest buildIsMasterRunningRequest() { } /** - * Creates a protocol buffer BalanceRequest - * + * Creates a protocol buffer BalanceRequest from a client BalanceRequest + * @param request * @return a BalanceRequest */ - public static BalanceRequest buildBalanceRequest(boolean force) { - return BalanceRequest.newBuilder().setForce(force).build(); + public static MasterProtos.BalanceRequest toBalanceRequest(BalanceRequest request) { + return MasterProtos.BalanceRequest.newBuilder() + .setDryRun(request.isDryRun()) + .setIgnoreRit(request.isIgnoreRegionsInTransition()) + .build(); + } + + public static BalanceRequest toBalanceRequest(MasterProtos.BalanceRequest request) { Review comment: I think this method is out of place -- the class comment says it is a utility class to build protocol buffer instance from POJOs. Do I have that wrong? ########## File path: hbase-protocol-shaded/src/main/protobuf/Master.proto ########## @@ -292,7 +292,8 @@ message IsInMaintenanceModeResponse { } message BalanceRequest { - optional bool force = 1; + optional bool ignore_rit = 1; Review comment: Can we rename these fields and have backward compatibility with old servers? I suppose it's okay as long as the wire protocol is positional only... Does "ignore_rit" mean strictly the same thing as "force"? ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/BalanceRequest.java ########## @@ -0,0 +1,96 @@ +/* + * + * 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; + +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.yetus.audience.InterfaceStability; + +/** + * Encapsulates options for executing an unscheduled run of the Balancer. + */ [email protected] [email protected] +public class BalanceRequest { + + private final boolean dryRun; + private final boolean ignoreRIT; + + private BalanceRequest(boolean dryRun, boolean ignoreRIT) { + this.dryRun = dryRun; + this.ignoreRIT = ignoreRIT; + } + + /** + * Creates a BalancerRequest which runs the balancer normally. + * In this mode, the balancer will execute all region moves if an improved + * plan is found. + * + * The balancer will not run if the balance switch is disabled, the master + * is in maintenance mode, or there are currently regions in transition. You + * can force the balancer to run despite regions being in transition by + * using {@link #ignoreRegionsInTransition()} + */ + public static BalanceRequest execute() { + return new BalanceRequest(false, false); + } + + + /** + * Creates a BalancerRequest which runs the balancer in dryRun mode. + * In this mode, the balancer will try to find a plan but WILL NOT + * execute any region moves or call any coprocessors. + * + * You can run in dryRun mode regardless of whether the balancer switch + * is enabled or disabled, but dryRun mode will not run over an existing + * request or chore. + * + * Dry run is useful for testing out new balance configs. See the logs + * on the active HMaster for the results of the dry run. + */ + public static BalanceRequest dryRun() { + return new BalanceRequest(true, false); + } + + /** + * For internal use only. Used to support deprecated interface in Admin/AsyncAdmin. + */ + @InterfaceAudience.Private + public static BalanceRequest execute(boolean force) { + BalanceRequest request = BalanceRequest.execute(); + return force ? request.ignoreRegionsInTransition() : request; + } + + /** + * Updates the BalancerRequest to cause the balancer to run even if there + * are regions in transition. + * + * WARNING: Advanced usage only, this could cause more issues than it fixes. + */ + public BalanceRequest ignoreRegionsInTransition() { + return new BalanceRequest(dryRun, true); + } + + public boolean isDryRun() { + return dryRun; + } + + public boolean isIgnoreRegionsInTransition() { Review comment: nit: it's surprising that the name of the accessor method in a POJO does not match the underlying variable name. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/MasterRpcServices.java ########## @@ -17,6 +17,7 @@ */ package org.apache.hadoop.hbase.master; +import static org.apache.hadoop.hbase.BalanceRequest.*; Review comment: is this used? ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java ########## @@ -1785,7 +1793,7 @@ public boolean balance(boolean force) throws IOException { return false; } - if (this.cpHost != null) { + if (this.cpHost != null && !request.isDryRun()) { Review comment: same question about skipping coprocessors. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/shaded/protobuf/RequestConverter.java ########## @@ -1577,12 +1577,20 @@ public static IsMasterRunningRequest buildIsMasterRunningRequest() { } /** - * Creates a protocol buffer BalanceRequest - * + * Creates a protocol buffer BalanceRequest from a client BalanceRequest + * @param request * @return a BalanceRequest */ - public static BalanceRequest buildBalanceRequest(boolean force) { - return BalanceRequest.newBuilder().setForce(force).build(); + public static MasterProtos.BalanceRequest toBalanceRequest(BalanceRequest request) { + return MasterProtos.BalanceRequest.newBuilder() + .setDryRun(request.isDryRun()) + .setIgnoreRit(request.isIgnoreRegionsInTransition()) + .build(); + } + + public static BalanceRequest toBalanceRequest(MasterProtos.BalanceRequest request) { + BalanceRequest newRequest = request.getDryRun() ? BalanceRequest.dryRun() : BalanceRequest.execute(); Review comment: I think you need to use the `hasDryRun` and `hasIgnoreRit` methods to check if these fields are set on the `MasterProtos.BalanceRequest` instance, and then `getDryRun`, `getIgnoreRit` to access the values when they're present. Unless you explicitly want to rely on the protobuf default value behavior here... I'm not sure what that is, but I am pretty sure that's not a good idea from the perspective of long-term maintenance. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java ########## @@ -1809,6 +1817,10 @@ public boolean balance(boolean force) throws IOException { List<RegionPlan> plans = this.balancer.balanceCluster(assignments); + if (request.isDryRun()) { + return true; Review comment: same question about returning a hard-coded value here as i had with RSGroups feature. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/BalanceRequest.java ########## @@ -0,0 +1,96 @@ +/* + * + * 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; + +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.yetus.audience.InterfaceStability; + +/** + * Encapsulates options for executing an unscheduled run of the Balancer. + */ [email protected] [email protected] +public class BalanceRequest { + + private final boolean dryRun; + private final boolean ignoreRIT; + + private BalanceRequest(boolean dryRun, boolean ignoreRIT) { + this.dryRun = dryRun; + this.ignoreRIT = ignoreRIT; + } + + /** + * Creates a BalancerRequest which runs the balancer normally. + * In this mode, the balancer will execute all region moves if an improved + * plan is found. + * + * The balancer will not run if the balance switch is disabled, the master + * is in maintenance mode, or there are currently regions in transition. You + * can force the balancer to run despite regions being in transition by + * using {@link #ignoreRegionsInTransition()} + */ + public static BalanceRequest execute() { + return new BalanceRequest(false, false); + } + + + /** + * Creates a BalancerRequest which runs the balancer in dryRun mode. + * In this mode, the balancer will try to find a plan but WILL NOT + * execute any region moves or call any coprocessors. + * + * You can run in dryRun mode regardless of whether the balancer switch + * is enabled or disabled, but dryRun mode will not run over an existing + * request or chore. + * + * Dry run is useful for testing out new balance configs. See the logs + * on the active HMaster for the results of the dry run. + */ + public static BalanceRequest dryRun() { + return new BalanceRequest(true, false); + } + + /** + * For internal use only. Used to support deprecated interface in Admin/AsyncAdmin. + */ + @InterfaceAudience.Private Review comment: Is it strictly necessary to include an IA.Private method on this otherwise public interface object? It is my experience that keeping track of these little exceptions in our public API is more work than it's worth. I would prefer to see no method-level overrides to the class-declared IA visibility and have this method moved to where it's used, as an implementation detail. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/BalanceRequest.java ########## @@ -0,0 +1,96 @@ +/* + * + * 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; + +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.yetus.audience.InterfaceStability; + +/** + * Encapsulates options for executing an unscheduled run of the Balancer. + */ [email protected] [email protected] +public class BalanceRequest { + + private final boolean dryRun; Review comment: The wire protocol specifies both these fields as `optional`, yet this class requires values be provided by both of them. Is that intentional? I expected this class to faithfully mirror the structure of the wire protocol. ########## File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterDryRunBalancer.java ########## @@ -0,0 +1,116 @@ +/** + * 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; + +import static org.apache.hadoop.hbase.BalanceRequest.*; +import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertFalse; +import static org.junit.Assert.assertTrue; +import java.io.IOException; +import org.apache.hadoop.hbase.BalanceRequest; +import org.apache.hadoop.hbase.HBaseClassTestRule; +import org.apache.hadoop.hbase.HBaseTestingUtility; +import org.apache.hadoop.hbase.ServerName; +import org.apache.hadoop.hbase.TableName; +import org.apache.hadoop.hbase.Waiter; +import org.apache.hadoop.hbase.client.RegionInfo; +import org.apache.hadoop.hbase.regionserver.HRegionServer; +import org.apache.hadoop.hbase.testclassification.MasterTests; +import org.apache.hadoop.hbase.testclassification.MediumTests; +import org.apache.hadoop.hbase.util.Bytes; +import org.junit.After; +import org.junit.ClassRule; +import org.junit.Test; +import org.junit.experimental.categories.Category; +import org.mockito.Mockito; + +@Category({ MasterTests.class, MediumTests.class}) +public class TestMasterDryRunBalancer { + @ClassRule + public static final HBaseClassTestRule CLASS_RULE = + HBaseClassTestRule.forClass(TestMasterDryRunBalancer.class); + + private static final HBaseTestingUtility TEST_UTIL = new HBaseTestingUtility(); + private static final byte[] FAMILYNAME = Bytes.toBytes("fam"); + + @After + public void shutdown() throws Exception { + TEST_UTIL.shutdownMiniCluster(); + } + + @Test + public void testDryRunBalancer() throws Exception { + TEST_UTIL.startMiniCluster(2); + + TableName tableName = createTable("testDryRunBalancer"); + HMaster master = Mockito.spy(TEST_UTIL.getHBaseCluster().getMaster()); + + // dry run should be possible with balancer disabled + // disabling it will ensure the chore does not mess with our forced unbalance below + master.balanceSwitch(false); + assertFalse(master.isBalancerOn()); + + HRegionServer biasedServer = unbalance(master, tableName); + + assertTrue(master.balance(BalanceRequest.dryRun())); + + // sanity check that we truly don't try to execute any plans + Mockito.verify(master, Mockito.never()).executeRegionPlansWithThrottling(Mockito.anyList()); + + // should still be unbalanced post dry run + assertServerContainsAllRegions(biasedServer.getServerName(), tableName); + + TEST_UTIL.deleteTable(tableName); + } + + private TableName createTable(String table) throws IOException { + TableName tableName = TableName.valueOf(table); + TEST_UTIL.createMultiRegionTable(tableName, FAMILYNAME, 100); + return tableName; + } + + + private HRegionServer unbalance(HMaster master, TableName tableName) throws Exception { + waitForRegionsToSettle(master); + + HRegionServer biasedServer = TEST_UTIL.getMiniHBaseCluster().getRegionServer(0); + + for (RegionInfo regionInfo : TEST_UTIL.getAdmin().getRegions(tableName)) { + master.move(regionInfo.getEncodedNameAsBytes(), + Bytes.toBytes(biasedServer.getServerName().getServerName())); + } + + waitForRegionsToSettle(master); + + assertServerContainsAllRegions(biasedServer.getServerName(), tableName); + + return biasedServer; + } + + private void assertServerContainsAllRegions(ServerName serverName, TableName tableName) + throws IOException { + int numRegions = TEST_UTIL.getAdmin().getRegions(tableName).size(); + assertEquals(numRegions, + TEST_UTIL.getMiniHBaseCluster().getRegionServer(serverName).getRegions(tableName).size()); + } + + private void waitForRegionsToSettle(HMaster master) { + Waiter.waitFor(TEST_UTIL.getConfiguration(), 15_000, Review comment: 15 seconds is likely not long enough for this test to run reliably on apache jenkins. ########## File path: hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminClient.java ########## @@ -27,6 +27,7 @@ import java.util.Set; import java.util.stream.Collectors; +import org.apache.hadoop.hbase.BalanceRequest; Review comment: There's no `BalanceRSGroupRequest` ? I'm surprised to see a wire object without a POJO for use on the client. Reusing the POJO of one RPC for the other means the two APIs cannot evolve independently. We should not do this. ########## File path: hbase-client/src/main/java/org/apache/hadoop/hbase/BalanceRequest.java ########## @@ -0,0 +1,96 @@ +/* + * + * 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; + +import org.apache.yetus.audience.InterfaceAudience; +import org.apache.yetus.audience.InterfaceStability; + +/** + * Encapsulates options for executing an unscheduled run of the Balancer. + */ [email protected] [email protected] +public class BalanceRequest { + + private final boolean dryRun; + private final boolean ignoreRIT; + + private BalanceRequest(boolean dryRun, boolean ignoreRIT) { + this.dryRun = dryRun; + this.ignoreRIT = ignoreRIT; + } + + /** + * Creates a BalancerRequest which runs the balancer normally. + * In this mode, the balancer will execute all region moves if an improved + * plan is found. + * + * The balancer will not run if the balance switch is disabled, the master + * is in maintenance mode, or there are currently regions in transition. You + * can force the balancer to run despite regions being in transition by + * using {@link #ignoreRegionsInTransition()} + */ + public static BalanceRequest execute() { + return new BalanceRequest(false, false); + } + + + /** + * Creates a BalancerRequest which runs the balancer in dryRun mode. + * In this mode, the balancer will try to find a plan but WILL NOT + * execute any region moves or call any coprocessors. + * + * You can run in dryRun mode regardless of whether the balancer switch + * is enabled or disabled, but dryRun mode will not run over an existing + * request or chore. + * + * Dry run is useful for testing out new balance configs. See the logs + * on the active HMaster for the results of the dry run. + */ + public static BalanceRequest dryRun() { + return new BalanceRequest(true, false); + } + + /** + * For internal use only. Used to support deprecated interface in Admin/AsyncAdmin. + */ + @InterfaceAudience.Private + public static BalanceRequest execute(boolean force) { + BalanceRequest request = BalanceRequest.execute(); + return force ? request.ignoreRegionsInTransition() : request; + } + + /** + * Updates the BalancerRequest to cause the balancer to run even if there + * are regions in transition. + * + * WARNING: Advanced usage only, this could cause more issues than it fixes. + */ + public BalanceRequest ignoreRegionsInTransition() { Review comment: Why is this method here? Either have mutable fields with setters or have an exhaustive set of static constructors. This constructor behind an existing instance is not really idiomatic for a POJO. ########## File path: hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupProtobufUtil.java ########## @@ -36,6 +38,19 @@ private RSGroupProtobufUtil() { } + static BalanceRSGroupRequest createBalanceRSGroupRequest(String groupName, BalanceRequest request) { + return BalanceRSGroupRequest.newBuilder() + .setRSGroupName(groupName) + .setDryRun(request.isDryRun()) + .setIgnoreRit(request.isIgnoreRegionsInTransition()) + .build(); + } + + static BalanceRequest toBalanceRequest(BalanceRSGroupRequest request) { + BalanceRequest newRequest = request.getDryRun() ? BalanceRequest.dryRun() : BalanceRequest.execute(); Review comment: Same questions about `hasFoo` vs. `getFoo`. ########## File path: hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminEndpoint.java ########## @@ -291,17 +292,19 @@ public void removeRSGroup(RpcController controller, @Override public void balanceRSGroup(RpcController controller, BalanceRSGroupRequest request, RpcCallback<BalanceRSGroupResponse> done) { + BalanceRequest balanceRequest = RSGroupProtobufUtil.toBalanceRequest(request); + BalanceRSGroupResponse.Builder builder = BalanceRSGroupResponse.newBuilder(); LOG.info(master.getClientIdAuditPrefix() + " balance rsgroup, group=" + request.getRSGroupName()); try { - if (master.getMasterCoprocessorHost() != null) { + if (master.getMasterCoprocessorHost() != null && !balanceRequest.isDryRun()) { Review comment: Why skip the coprocessor call-back? Shouldn't the coprocessor be informed with the full content of the request object? ########## File path: hbase-rsgroup/src/main/java/org/apache/hadoop/hbase/rsgroup/RSGroupAdminServer.java ########## @@ -541,6 +544,11 @@ public boolean balanceRSGroup(String groupName) throws IOException { getRSGroupAssignmentsByTable(master.getTableStateManager(), groupName); List<RegionPlan> plans = balancer.balanceCluster(assignmentsByTable); boolean balancerRan = !plans.isEmpty(); + + if (request.isDryRun()) { + return balancerRan; Review comment: Is this correct? The balancer will never run when `dry_run=true`. Why does the response indicate otherwise? I guess this boolean response is not rich enough to communicate that a plan was calculated but not applied to the cluster. ########## File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java ########## @@ -1750,10 +1751,16 @@ public boolean skipRegionManagementAction(final String action) { return false; } - public boolean balance(boolean force) throws IOException { - if (loadBalancerTracker == null || !loadBalancerTracker.isBalancerOn()) { + public boolean balance(BalanceRequest request) throws IOException { + return balance(request, false); + } + + private boolean balance(BalanceRequest request, boolean isChore) throws IOException { Review comment: What's this new business about `isChore`? -- 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]
