[GitHub] [hbase] joshelser commented on a change in pull request #3536: HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves
joshelser commented on a change in pull request #3536:
URL: https://github.com/apache/hbase/pull/3536#discussion_r696061323
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/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.client;
+
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+
+/**
+ * Encapsulates options for executing a run of the Balancer.
+ */
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public final class BalanceRequest {
+ private static final BalanceRequest DEFAULT =
BalanceRequest.newBuilder().build();
+
+ @InterfaceAudience.Public
+ @InterfaceStability.Evolving
+ public final static class Builder {
Review comment:
Should have some javadoc on the Builder too
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/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.client;
+
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+
+/**
+ * Encapsulates options for executing a run of the Balancer.
+ */
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public final class BalanceRequest {
Review comment:
We should have javadoc on public methods in `IA.Public` classes
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalanceResponse.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.client;
+
+import org.apache.yetus.audience.InterfaceAudience;
+import org.apache.yetus.audience.InterfaceStability;
+
+/**
+ * Response returned from a balancer invocation
+ */
+@InterfaceAudience.Public
+@InterfaceStability.Evolving
+public final class BalanceResponse {
+
+ @InterfaceAudience.Public
+ @InterfaceStability.Evolving
+ public final static class Builder {
Review comment:
Javadoc on builder too, please.
##
File path:
hbase-client/src/main/java/org/apache/hadoop/hbase/client/BalanceResponse.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
[GitHub] [hbase] joshelser commented on a change in pull request #3536: HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves
joshelser commented on a change in pull request #3536:
URL: https://github.com/apache/hbase/pull/3536#discussion_r682925736
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -1253,6 +1253,15 @@ default boolean balancer() throws IOException {
*/
boolean balance() throws IOException;
+ /**
+ * Invoke the balancer in dry run mode. Will show plan but not actually move
any regions.
+ * Can NOT run for various reasons. Check logs.
+ *
+ * @return true if dry run ran, false otherwise.
+ * @throws IOException if a remote or network exception occurs
+ */
+ boolean dryRunBalance() throws IOException;
Review comment:
> I would vote for BalancerConfig. We know that we want to add support
for config overrides, and it'd be awkward to add that aside an enum or boolean.
Yeah, I think this is the nicer of the options. This lets us avoid making
big changes to `Admin` itself, but limit those to an `Evolving`
`BalancerConfig` object. We provide a bit more stability to `Admin` that way,
IMO.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] joshelser commented on a change in pull request #3536: HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves
joshelser commented on a change in pull request #3536:
URL: https://github.com/apache/hbase/pull/3536#discussion_r682925736
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -1253,6 +1253,15 @@ default boolean balancer() throws IOException {
*/
boolean balance() throws IOException;
+ /**
+ * Invoke the balancer in dry run mode. Will show plan but not actually move
any regions.
+ * Can NOT run for various reasons. Check logs.
+ *
+ * @return true if dry run ran, false otherwise.
+ * @throws IOException if a remote or network exception occurs
+ */
+ boolean dryRunBalance() throws IOException;
Review comment:
> I would vote for BalancerConfig. We know that we want to add support
for config overrides, and it'd be awkward to add that aside an enum or boolean.
Yeah, I think this is the nicer of the options. This lets us avoid making
big changes to `Admin` itself, but limit those to an `Evolving`
`BalancerConfig` object. We provide a bit more stability to `Admin` that way,
IMO.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] joshelser commented on a change in pull request #3536: HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves
joshelser commented on a change in pull request #3536:
URL: https://github.com/apache/hbase/pull/3536#discussion_r679486521
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/HMaster.java
##
@@ -1809,6 +1809,10 @@ public boolean balance(boolean force) throws IOException
{
List plans = this.balancer.balanceCluster(assignments);
+ if (runMode.isDryRun()) {
+return true;
Review comment:
Heh, yeah, I saw the chatter on Jira after I posted the review 蘿
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] joshelser commented on a change in pull request #3536: HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves
joshelser commented on a change in pull request #3536:
URL: https://github.com/apache/hbase/pull/3536#discussion_r679486302
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -1253,6 +1253,15 @@ default boolean balancer() throws IOException {
*/
boolean balance() throws IOException;
+ /**
+ * Invoke the balancer in dry run mode. Will show plan but not actually move
any regions.
+ * Can NOT run for various reasons. Check logs.
+ *
+ * @return true if dry run ran, false otherwise.
+ * @throws IOException if a remote or network exception occurs
+ */
+ boolean dryRunBalance() throws IOException;
Review comment:
> I'm not sure how the hbase devs weigh interface change questions like
this.
Yeah, that's exactly the problem ;). As you have added it now, you've added
a new method to the public API which we would not be able to remove before
HBase 4.0.0 at this point. Meaning, we want to be very cautious when adding new
methods on public API.
I believe our specification technically allow us to define a single method
on the interface to be at a lesser visibility (not Public), but it's not
something we typically do, especially on one of the most user-facing interfaces.
--
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: issues-unsubscr...@hbase.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
[GitHub] [hbase] joshelser commented on a change in pull request #3536: HBASE-26147: Add dry_run_balancer and related Admin interfaces for running the balancer without executing any region moves
joshelser commented on a change in pull request #3536:
URL: https://github.com/apache/hbase/pull/3536#discussion_r679453610
##
File path: hbase-client/src/main/java/org/apache/hadoop/hbase/client/Admin.java
##
@@ -1253,6 +1253,15 @@ default boolean balancer() throws IOException {
*/
boolean balance() throws IOException;
+ /**
+ * Invoke the balancer in dry run mode. Will show plan but not actually move
any regions.
+ * Can NOT run for various reasons. Check logs.
+ *
+ * @return true if dry run ran, false otherwise.
+ * @throws IOException if a remote or network exception occurs
+ */
+ boolean dryRunBalance() throws IOException;
Review comment:
I'm not a big fan of adding a brand new API just for the `dry-run` mode.
What about overloading the existing `balance()` method?
##
File path:
hbase-server/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java
##
@@ -86,6 +86,18 @@
// We deliberately use 'localhost' so the operation will fail fast
ServerName BOGUS_SERVER_NAME = ServerName.valueOf("localhost,1,1");
+ enum RunMode {
+CHORE, REQUEST, FORCE, DRY_RUN;
+
+boolean isForced() {
+ return this == FORCE;
+}
+
+boolean isDryRun() {
+ return this == DRY_RUN;
+}
Review comment:
IMO, these aren't adding much over `runMode == RunMode.DRY_RUN` and
`runMode == RunMode.FORCE`.
##
File path:
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestMasterDryRunBalancer.java
##
@@ -0,0 +1,114 @@
+/**
+ * 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.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+import java.io.IOException;
+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.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(LoadBalancer.RunMode.DRY_RUN));
+
+// 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);
+byte[] startKey = new byte[] { 0x00 };
+byte[] stopKey = new byte[] { 0x7f };
+TEST_UTIL.createTable(tableName, new byte[][] { FAMILYNAME }, 1, startKey,
stopKey,
+ 100);
+return tableName;
+ }
+
+
+ private
