You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by "rmdmattingly (via GitHub)" <gi...@apache.org> on 2025/03/19 13:11:53 UTC
[PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
rmdmattingly opened a new pull request, #6821:
URL: https://github.com/apache/hbase/pull/6821
Because we'll accept any moves that improve conditional violations, this bug can cause us to erroneously approve balancer actions.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #6821:
URL: https://github.com/apache/hbase/pull/6821#issuecomment-2741311423
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 38s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck |
|||| _ Prechecks _ |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 2s | | master passed |
| +1 :green_heart: | compile | 0m 30s | | master passed |
| +1 :green_heart: | javadoc | 0m 17s | | master passed |
| +1 :green_heart: | shadedjars | 7m 59s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 38s | | the patch passed |
| +1 :green_heart: | compile | 0m 24s | | the patch passed |
| +1 :green_heart: | javac | 0m 24s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 19s | | the patch passed |
| +1 :green_heart: | shadedjars | 8m 1s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 8m 42s | | hbase-balancer in the patch passed. |
| | | 36m 38s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/2/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/6821 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux 308b0b617a94 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 62267300f31edd4f4dc010c4a10ff57aa3313a1a |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/2/testReport/ |
| Max. process+thread count | 269 (vs. ulimit of 30000) |
| modules | C: hbase-balancer U: hbase-balancer |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/2/console |
| versions | git=2.34.1 maven=3.9.8 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "rmdmattingly (via GitHub)" <gi...@apache.org>.
rmdmattingly commented on code in PR #6821:
URL: https://github.com/apache/hbase/pull/6821#discussion_r2006144309
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/CandidateGeneratorTestUtil.java:
##########
@@ -71,7 +77,7 @@ static void runBalancerToExhaustion(Configuration conf,
boolean isBalanced = false;
while (!isBalanced) {
balancerRuns++;
- if (balancerRuns > 1000) {
+ if (balancerRuns > 10) {
Review Comment:
I validated locally that this change is okay, will see if the tests also pass in CI. If it's taking more than 10 balancer runs for a test, then we should probably increase that tests max run time to make it more efficient
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "rmdmattingly (via GitHub)" <gi...@apache.org>.
rmdmattingly commented on code in PR #6821:
URL: https://github.com/apache/hbase/pull/6821#discussion_r2006143463
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestUnattainableBalancerCostGoal.java:
##########
@@ -0,0 +1,108 @@
+/*
+ * 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.MasterTests;
+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;
+
+/**
+ * If your minCostNeedsBalance is set too low, then the balancer should still eventually stop making
+ * moves as further cost improvements become impossible, and balancer plan calculation becomes
+ * wasteful. This test ensures that the balancer will not get stuck in a loop of continuously moving
+ * regions.
+ */
+@Category({ MasterTests.class, MediumTests.class })
+public class TestUnattainableBalancerCostGoal {
Review Comment:
This test fails on master atm (if balancer conditionals are enabled). It passes with this patch
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "ndimiduk (via GitHub)" <gi...@apache.org>.
ndimiduk commented on code in PR #6821:
URL: https://github.com/apache/hbase/pull/6821#discussion_r2003304664
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerConditionals.java:
##########
@@ -145,7 +145,7 @@ int getViolationCountChange(BalancerClusterState cluster, BalanceAction action)
// Reset cluster
cluster.doAction(undoAction);
- if (isViolatingPre && isViolatingPost) {
+ if (isViolatingPre == isViolatingPost) {
return 0;
Review Comment:
Since this was so impactful to behavior, maybe good to capture this method in a unit test?
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "rmdmattingly (via GitHub)" <gi...@apache.org>.
rmdmattingly merged PR #6821:
URL: https://github.com/apache/hbase/pull/6821
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #6821:
URL: https://github.com/apache/hbase/pull/6821#issuecomment-2741296770
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 32s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any anti-patterns. |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 26s | | master passed |
| +1 :green_heart: | compile | 0m 27s | | master passed |
| +1 :green_heart: | checkstyle | 0m 10s | | master passed |
| +1 :green_heart: | spotbugs | 0m 27s | | master passed |
| +1 :green_heart: | spotless | 0m 47s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 7s | | the patch passed |
| +1 :green_heart: | compile | 0m 25s | | the patch passed |
| +1 :green_heart: | javac | 0m 25s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 8s | | the patch passed |
| +1 :green_heart: | spotbugs | 0m 31s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 12m 6s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.0. |
| +1 :green_heart: | spotless | 0m 45s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 9s | | The patch does not generate ASF License warnings. |
| | | 30m 29s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/6821 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 65c9c347c535 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 62267300f31edd4f4dc010c4a10ff57aa3313a1a |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 81 (vs. ulimit of 30000) |
| modules | C: hbase-balancer U: hbase-balancer |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/2/console |
| versions | git=2.34.1 maven=3.9.8 spotbugs=4.7.3 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #6821:
URL: https://github.com/apache/hbase/pull/6821#issuecomment-2737297544
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 0m 28s | | Docker mode activated. |
|||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | | No case conflicting files found. |
| +0 :ok: | codespell | 0m 0s | | codespell was not available. |
| +0 :ok: | detsecrets | 0m 0s | | detect-secrets was not available. |
| +1 :green_heart: | @author | 0m 0s | | The patch does not contain any @author tags. |
| +1 :green_heart: | hbaseanti | 0m 0s | | Patch does not have any anti-patterns. |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 20s | | master passed |
| +1 :green_heart: | compile | 0m 26s | | master passed |
| +1 :green_heart: | checkstyle | 0m 10s | | master passed |
| +1 :green_heart: | spotbugs | 0m 26s | | master passed |
| +1 :green_heart: | spotless | 0m 45s | | branch has no errors when running spotless:check. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 3m 3s | | the patch passed |
| +1 :green_heart: | compile | 0m 24s | | the patch passed |
| +1 :green_heart: | javac | 0m 24s | | the patch passed |
| +1 :green_heart: | blanks | 0m 0s | | The patch has no blanks issues. |
| +1 :green_heart: | checkstyle | 0m 9s | | the patch passed |
| +1 :green_heart: | spotbugs | 0m 30s | | the patch passed |
| +1 :green_heart: | hadoopcheck | 11m 58s | | Patch does not cause any errors with Hadoop 3.3.6 3.4.0. |
| +1 :green_heart: | spotless | 0m 45s | | patch has no errors when running spotless:check. |
|||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 10s | | The patch does not generate ASF License warnings. |
| | | 29m 54s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/6821 |
| Optional Tests | dupname asflicense javac spotbugs checkstyle codespell detsecrets compile hadoopcheck hbaseanti spotless |
| uname | Linux 79490f681567 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / e21aaafb66a202a0e1b38aa2cd90e200d4553cd6 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Max. process+thread count | 82 (vs. ulimit of 30000) |
| modules | C: hbase-balancer U: hbase-balancer |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/1/console |
| versions | git=2.34.1 maven=3.9.8 spotbugs=4.7.3 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "rmdmattingly (via GitHub)" <gi...@apache.org>.
rmdmattingly commented on code in PR #6821:
URL: https://github.com/apache/hbase/pull/6821#discussion_r2003298744
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BalancerConditionals.java:
##########
@@ -145,7 +145,7 @@ int getViolationCountChange(BalancerClusterState cluster, BalanceAction action)
// Reset cluster
cluster.doAction(undoAction);
- if (isViolatingPre && isViolatingPost) {
+ if (isViolatingPre == isViolatingPost) {
return 0;
Review Comment:
We also want to return 0 if we had zero violations both pre & post. Prior to this fix, we'd fall through to return -1 which erroneously considered a neutral move to be an improvement
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "Apache-HBase (via GitHub)" <gi...@apache.org>.
Apache-HBase commented on PR #6821:
URL: https://github.com/apache/hbase/pull/6821#issuecomment-2737426394
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Logfile | Comment |
|:----:|----------:|--------:|:--------:|:-------:|
| +0 :ok: | reexec | 4m 51s | | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --author-ignore-list --blanks-eol-ignore-file --blanks-tabs-ignore-file --quick-hadoopcheck |
|||| _ Prechecks _ |
|||| _ master Compile Tests _ |
| +1 :green_heart: | mvninstall | 5m 2s | | master passed |
| +1 :green_heart: | compile | 2m 6s | | master passed |
| +1 :green_heart: | javadoc | 0m 37s | | master passed |
| +1 :green_heart: | shadedjars | 8m 13s | | branch has no errors when building our shaded downstream artifacts. |
|||| _ Patch Compile Tests _ |
| +1 :green_heart: | mvninstall | 4m 34s | | the patch passed |
| +1 :green_heart: | compile | 0m 20s | | the patch passed |
| +1 :green_heart: | javac | 0m 20s | | the patch passed |
| +1 :green_heart: | javadoc | 0m 18s | | the patch passed |
| +1 :green_heart: | shadedjars | 7m 2s | | patch has no errors when building our shaded downstream artifacts. |
|||| _ Other Tests _ |
| +1 :green_heart: | unit | 8m 30s | | hbase-balancer in the patch passed. |
| | | 43m 12s | | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | ClientAPI=1.43 ServerAPI=1.43 base: https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/1/artifact/yetus-jdk17-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/6821 |
| Optional Tests | javac javadoc unit compile shadedjars |
| uname | Linux cc8f1f487177 5.4.0-1103-aws #111~18.04.1-Ubuntu SMP Tue May 23 20:04:10 UTC 2023 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / e21aaafb66a202a0e1b38aa2cd90e200d4553cd6 |
| Default Java | Eclipse Adoptium-17.0.11+9 |
| Test Results | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/1/testReport/ |
| Max. process+thread count | 273 (vs. ulimit of 30000) |
| modules | C: hbase-balancer U: hbase-balancer |
| Console output | https://ci-hbase.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-6821/1/console |
| versions | git=2.34.1 maven=3.9.8 |
| Powered by | Apache Yetus 0.15.0 https://yetus.apache.org |
This message was automatically generated.
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] HBASE-29202 Balancer conditionals make balancer actions more likely to be approved [hbase]
Posted by "ndimiduk (via GitHub)" <gi...@apache.org>.
ndimiduk commented on code in PR #6821:
URL: https://github.com/apache/hbase/pull/6821#discussion_r2007170754
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/CandidateGeneratorTestUtil.java:
##########
@@ -46,16 +46,22 @@ public final class CandidateGeneratorTestUtil {
private CandidateGeneratorTestUtil() {
}
+ enum ExhaustionType {
Review Comment:
👍
--
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-unsubscribe@hbase.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org