You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@hbase.apache.org by GitBox <gi...@apache.org> on 2020/07/10 07:39:21 UTC
[GitHub] [hbase] bsglz opened a new pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
bsglz opened a new pull request #2044:
URL: https://github.com/apache/hbase/pull/2044
…k hours
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660478607
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 31s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 22s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 8s | master passed |
| +1 :green_heart: | compile | 2m 45s | master passed |
| +1 :green_heart: | shadedjars | 5m 43s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 17s | root in master failed. |
| -0 :warning: | javadoc | 0m 38s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 56s | the patch passed |
| +1 :green_heart: | compile | 2m 46s | the patch passed |
| +1 :green_heart: | javac | 2m 46s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 45s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 39s | hbase-server in the patch failed. |
| -0 :warning: | javadoc | 0m 15s | root in the patch failed. |
||| _ Other Tests _ |
| -1 :x: | unit | 163m 57s | root in the patch failed. |
| | | 195m 7s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 9254a5ab6f7c 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/testReport/ |
| Max. process+thread count | 6394 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] liuml07 commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
liuml07 commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660431436
> Here need conf also: OffPeakHours.getInstance(conf).isOffPeakHour()
Right. Unless we also save `OffPeakHours` instance in constructor...so the value of removing `conf` need much effort. We can stay with current implementation I guess then.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660410619
The prob is that here we want change the multiplier everyday when the current hour switch between peak and offpeak, there will not a config changed event.
Thanks. @virajjasani
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani closed pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani closed pull request #2044:
URL: https://github.com/apache/hbase/pull/2044
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] liuml07 commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
liuml07 commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456750615
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
Review comment:
@bsglz and @virajjasani I do not have preference here if you agree on that. Either way works obviously!
I asked again (see [later comment](https://github.com/apache/hbase/pull/2044#discussion_r456748462)) because I'm not sure this is entirely true:
> If default values change here and test is not updated, it is anyways going to fail.
Suppose there is a bug in code (not today but future). The bug will be hidden if test still pass (possible) when we update the default value and code, but not the test.
Another pro of using variable `DEFAULT_MOVE_COST` instead of literal value `7` is about readability. Clearer to know where the test expected value comes from. The main con of using `@VisibleForTesting` is about exposing private values, which I personally always avoid... I can see the value of either way here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660499543
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 34s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 23s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 38s | master passed |
| +1 :green_heart: | compile | 2m 21s | master passed |
| +1 :green_heart: | shadedjars | 5m 34s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 37s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 24s | the patch passed |
| +1 :green_heart: | compile | 2m 17s | the patch passed |
| +1 :green_heart: | javac | 2m 17s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 34s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 36s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 326m 27s | root in the patch failed. |
| | | 358m 12s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 886d7460b2ca 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| Default Java | 1.8.0_232 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/testReport/ |
| Max. process+thread count | 5627 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660522994
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 22s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 9s | master passed |
| +1 :green_heart: | compile | 2m 45s | master passed |
| +1 :green_heart: | shadedjars | 5m 47s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 17s | root in master failed. |
| -0 :warning: | javadoc | 0m 39s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 4s | the patch passed |
| +1 :green_heart: | compile | 2m 44s | the patch passed |
| +1 :green_heart: | javac | 2m 44s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 40s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 39s | hbase-server in the patch failed. |
| -0 :warning: | javadoc | 0m 15s | root in the patch failed. |
||| _ Other Tests _ |
| -1 :x: | unit | 164m 9s | root in the patch failed. |
| | | 195m 18s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 5ce5d49295ee 4.15.0-58-generic #64-Ubuntu SMP Tue Aug 6 11:12:41 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/testReport/ |
| Max. process+thread count | 6565 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660507680
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 32s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 22s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 8s | master passed |
| +1 :green_heart: | checkstyle | 2m 20s | master passed |
| +0 :ok: | refguide | 5m 33s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | spotbugs | 13m 25s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 43s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 30s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 6m 33s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | hadoopcheck | 13m 20s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 14m 22s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 25s | The patch does not generate ASF License warnings. |
| | | 75m 47s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle refguide |
| uname | Linux fec8c79a9ecf 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-general-check/output/branch-site/book.html |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-general-check/output/patch-site/book.html |
| Max. process+thread count | 122 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456748565
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java
##########
@@ -284,6 +284,25 @@ public void testLocalityCost() throws Exception {
}
}
+ @Test
+ public void testMoveCostMultiplier() throws Exception {
+ Configuration conf = HBaseConfiguration.create();
+ StochasticLoadBalancer.CostFunction
+ costFunction = new StochasticLoadBalancer.MoveCostFunction(conf);
+ BaseLoadBalancer.Cluster cluster = mockCluster(clusterStateMocks[0]);
+ costFunction.init(cluster);
+ costFunction.cost();
+ assertEquals(7, costFunction.getMultiplier(), 0.01);
Review comment:
@liuml07 Yeah, you are right. Thanks for the 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r452891195
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
private static final int DEFAULT_MAX_MOVES = 600;
private static final float DEFAULT_MAX_MOVE_PERCENT = 0.25f;
private final float maxMovesPercent;
+ private final Configuration conf;
MoveCostFunction(Configuration conf) {
super(conf);
-
- // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
- // that large benefits are need to overcome the cost of a move.
- this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ this.conf = conf;
// What percent of the number of regions a single run of the balancer can move.
maxMovesPercent = conf.getFloat(MAX_MOVES_PERCENT_KEY, DEFAULT_MAX_MOVE_PERCENT);
}
@Override
protected double cost() {
+ // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
+ // that large benefits are need to overcome the cost of a move.
+ if (OffPeakHours.getInstance(conf).isOffPeakHour()) {
+ this.setMultiplier(conf.getFloat(MOVE_COST_OFFPEAK_KEY, DEFAULT_MOVE_COST_OFFPEAK));
+ } else {
+ this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ }
Review comment:
The blancer only be initialized once on start, but we want the multiplier changed at offpeak hours.
Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456748462
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java
##########
@@ -284,6 +284,25 @@ public void testLocalityCost() throws Exception {
}
}
+ @Test
+ public void testMoveCostMultiplier() throws Exception {
+ Configuration conf = HBaseConfiguration.create();
+ StochasticLoadBalancer.CostFunction
+ costFunction = new StochasticLoadBalancer.MoveCostFunction(conf);
+ BaseLoadBalancer.Cluster cluster = mockCluster(clusterStateMocks[0]);
+ costFunction.init(cluster);
+ costFunction.cost();
+ assertEquals(7, costFunction.getMultiplier(), 0.01);
Review comment:
> Can we please avoid @VisibleForTesting and make them private? On the other hand, use default value directly in test cases. If default values change here and test is not updated, it is anyways going to fail.
Discussed about it above with viraj.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r452910042
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
private static final int DEFAULT_MAX_MOVES = 600;
private static final float DEFAULT_MAX_MOVE_PERCENT = 0.25f;
private final float maxMovesPercent;
+ private final Configuration conf;
MoveCostFunction(Configuration conf) {
super(conf);
-
- // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
- // that large benefits are need to overcome the cost of a move.
- this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ this.conf = conf;
// What percent of the number of regions a single run of the balancer can move.
maxMovesPercent = conf.getFloat(MAX_MOVES_PERCENT_KEY, DEFAULT_MAX_MOVE_PERCENT);
}
@Override
protected double cost() {
+ // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
+ // that large benefits are need to overcome the cost of a move.
+ if (OffPeakHours.getInstance(conf).isOffPeakHour()) {
+ this.setMultiplier(conf.getFloat(MOVE_COST_OFFPEAK_KEY, DEFAULT_MOVE_COST_OFFPEAK));
+ } else {
+ this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ }
Review comment:
Good.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660617317
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 27s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 38s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 59s | master passed |
| +1 :green_heart: | compile | 3m 12s | master passed |
| +1 :green_heart: | shadedjars | 6m 36s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 52s | hbase-server in master failed. |
| -0 :warning: | javadoc | 0m 14s | root in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 44s | the patch passed |
| +1 :green_heart: | compile | 3m 11s | the patch passed |
| +1 :green_heart: | javac | 3m 11s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 44s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 48s | hbase-server in the patch failed. |
| -0 :warning: | javadoc | 0m 16s | root in the patch failed. |
||| _ Other Tests _ |
| -1 :x: | unit | 263m 53s | root in the patch failed. |
| | | 299m 5s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 69be96bb7a15 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/testReport/ |
| Max. process+thread count | 4354 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-656658939
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 25s | Docker mode activated. |
| -0 :warning: | yetus | 0m 2s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 21s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 58s | master passed |
| +1 :green_heart: | compile | 3m 11s | master passed |
| +1 :green_heart: | shadedjars | 6m 41s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 17s | root in master failed. |
| -0 :warning: | javadoc | 0m 46s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 45s | the patch passed |
| +1 :green_heart: | compile | 3m 16s | the patch passed |
| +1 :green_heart: | javac | 3m 16s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 40s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 51s | hbase-server in the patch failed. |
| -0 :warning: | javadoc | 0m 13s | root in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 264m 10s | root in the patch passed. |
| | | 299m 8s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux e640fab0ccef 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 67dfbe0406 |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/testReport/ |
| Max. process+thread count | 4406 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456558482
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
private static final int DEFAULT_MAX_MOVES = 600;
private static final float DEFAULT_MAX_MOVE_PERCENT = 0.25f;
private final float maxMovesPercent;
+ private final Configuration conf;
MoveCostFunction(Configuration conf) {
super(conf);
-
- // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
- // that large benefits are need to overcome the cost of a move.
- this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ this.conf = conf;
// What percent of the number of regions a single run of the balancer can move.
maxMovesPercent = conf.getFloat(MAX_MOVES_PERCENT_KEY, DEFAULT_MAX_MOVE_PERCENT);
}
@Override
protected double cost() {
+ // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
+ // that large benefits are need to overcome the cost of a move.
+ if (OffPeakHours.getInstance(conf).isOffPeakHour()) {
+ this.setMultiplier(conf.getFloat(MOVE_COST_OFFPEAK_KEY, DEFAULT_MOVE_COST_OFFPEAK));
+ } else {
+ this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ }
Review comment:
@bsglz I just realized that whenever we update offpeak config dynamically, `MoveCostFunction` constructor is called so we should better have this logic in constructor only.
Check this out:
```
@Override
public void onConfigurationChange(Configuration conf) {
setConf(conf);
}
@Override
public synchronized void setConf(Configuration conf) {
super.setConf(conf);
...
...
costFunctions = new ArrayList<>();
costFunctions.add(new RegionCountSkewCostFunction(conf));
costFunctions.add(new PrimaryRegionCountSkewCostFunction(conf));
costFunctions.add(new MoveCostFunction(conf));
costFunctions.add(localityCost);
...
...
```
Hence, basically we can remove `this.conf = conf;` from this PR and move this if condition to constructor.
When we try to update the config dynamically for SLB, `onConfigurationChange()` is executed and that will reassign entire constFunctions list.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660427923
> In my last comment,
>
> > That said, to remove this.conf = conf dependency, we can get two multipliers from config, and call setMultiplier() accordingly here in cost() by checking isOffPeakHour().
>
> I meant in constructor, we can get two multipliers from config and remove the `this.conf` field. In `cost()` we call `setMultiplier()` and choose the right parameter by checking isOffPeakHour().
Here need conf also: OffPeakHours.getInstance(conf).isOffPeakHour()
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660572264
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 4m 52s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 23s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 16s | master passed |
| +1 :green_heart: | compile | 2m 53s | master passed |
| +1 :green_heart: | shadedjars | 6m 11s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 17s | root in master failed. |
| -0 :warning: | javadoc | 0m 42s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 9s | the patch passed |
| +1 :green_heart: | compile | 2m 50s | the patch passed |
| +1 :green_heart: | javac | 2m 50s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 50s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 44s | hbase-server in the patch failed. |
| -0 :warning: | javadoc | 0m 15s | root in the patch failed. |
||| _ Other Tests _ |
| -1 :x: | unit | 182m 4s | root in the patch failed. |
| | | 218m 24s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux da338d2f4f6f 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk11-hadoop3-check/output/patch-unit-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/testReport/ |
| Max. process+thread count | 6387 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-656756793
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 3s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 20s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 57s | master passed |
| +1 :green_heart: | checkstyle | 2m 16s | master passed |
| +0 :ok: | refguide | 5m 20s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | spotbugs | 11m 50s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 41s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 15s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 5m 32s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | hadoopcheck | 12m 14s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 12m 5s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 25s | The patch does not generate ASF License warnings. |
| | | 69m 18s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle refguide |
| uname | Linux aaa909b985ca 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 67dfbe0406 |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-general-check/output/branch-site/book.html |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-general-check/output/patch-site/book.html |
| Max. process+thread count | 122 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-659786007
@virajjasani Ping.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-657511102
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 6s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 24s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 5m 30s | master passed |
| +1 :green_heart: | compile | 3m 55s | master passed |
| +1 :green_heart: | shadedjars | 7m 48s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 18s | root in master failed. |
| -0 :warning: | javadoc | 0m 48s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 16s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 5m 32s | the patch passed |
| +1 :green_heart: | compile | 3m 41s | the patch passed |
| +1 :green_heart: | javac | 3m 41s | the patch passed |
| +1 :green_heart: | shadedjars | 7m 37s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 51s | hbase-server in the patch failed. |
| -0 :warning: | javadoc | 0m 16s | root in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 249m 59s | root in the patch passed. |
| | | 291m 16s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 044d62a6f085 4.15.0-91-generic #92-Ubuntu SMP Fri Feb 28 11:09:48 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / af1cc2fc44 |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/testReport/ |
| Max. process+thread count | 4483 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r452738026
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
Review comment:
Can we please avoid `@VisibleForTesting` and make them private? On the other hand, use default value directly in test cases. If default values change here and test is not updated, it is anyways going to fail.
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
private static final int DEFAULT_MAX_MOVES = 600;
private static final float DEFAULT_MAX_MOVE_PERCENT = 0.25f;
private final float maxMovesPercent;
+ private final Configuration conf;
MoveCostFunction(Configuration conf) {
super(conf);
-
- // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
- // that large benefits are need to overcome the cost of a move.
- this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ this.conf = conf;
// What percent of the number of regions a single run of the balancer can move.
maxMovesPercent = conf.getFloat(MAX_MOVES_PERCENT_KEY, DEFAULT_MAX_MOVE_PERCENT);
}
@Override
protected double cost() {
+ // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
+ // that large benefits are need to overcome the cost of a move.
+ if (OffPeakHours.getInstance(conf).isOffPeakHour()) {
+ this.setMultiplier(conf.getFloat(MOVE_COST_OFFPEAK_KEY, DEFAULT_MOVE_COST_OFFPEAK));
+ } else {
+ this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ }
Review comment:
Why not keep this logic in `MoveCostFunction` constructor?
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
Review comment:
nit: please limit 100 chars in one line
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-656568710
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 7s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 21s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 55s | master passed |
| +1 :green_heart: | checkstyle | 2m 17s | master passed |
| +0 :ok: | refguide | 5m 26s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | spotbugs | 11m 44s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 48s | the patch passed |
| -0 :warning: | checkstyle | 2m 19s | root: The patch generated 1 new + 60 unchanged - 0 fixed = 61 total (was 60) |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 5m 34s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | hadoopcheck | 12m 17s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 12m 17s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 26s | The patch does not generate ASF License warnings. |
| | | 69m 39s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle refguide |
| uname | Linux 3807de65804c 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 67dfbe0406 |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-general-check/output/branch-site/book.html |
| checkstyle | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-general-check/output/diff-checkstyle-root.txt |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-general-check/output/patch-site/book.html |
| Max. process+thread count | 122 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456751823
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
Review comment:
Seems the test will not pass when we update the default value?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] liuml07 commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
liuml07 commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456747846
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java
##########
@@ -284,6 +284,25 @@ public void testLocalityCost() throws Exception {
}
}
+ @Test
+ public void testMoveCostMultiplier() throws Exception {
+ Configuration conf = HBaseConfiguration.create();
+ StochasticLoadBalancer.CostFunction
+ costFunction = new StochasticLoadBalancer.MoveCostFunction(conf);
+ BaseLoadBalancer.Cluster cluster = mockCluster(clusterStateMocks[0]);
+ costFunction.init(cluster);
+ costFunction.cost();
+ assertEquals(7, costFunction.getMultiplier(), 0.01);
Review comment:
Not sure if we want to replace 7 with `DEFAULT_MOVE_COST`, which currently is private...same to `DEFAULT_MOVE_COST_OFFPEAK`
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
private static final int DEFAULT_MAX_MOVES = 600;
private static final float DEFAULT_MAX_MOVE_PERCENT = 0.25f;
private final float maxMovesPercent;
+ private final Configuration conf;
MoveCostFunction(Configuration conf) {
super(conf);
-
- // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
- // that large benefits are need to overcome the cost of a move.
- this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ this.conf = conf;
// What percent of the number of regions a single run of the balancer can move.
maxMovesPercent = conf.getFloat(MAX_MOVES_PERCENT_KEY, DEFAULT_MAX_MOVE_PERCENT);
}
@Override
protected double cost() {
+ // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
+ // that large benefits are need to overcome the cost of a move.
+ if (OffPeakHours.getInstance(conf).isOffPeakHour()) {
+ this.setMultiplier(conf.getFloat(MOVE_COST_OFFPEAK_KEY, DEFAULT_MOVE_COST_OFFPEAK));
+ } else {
+ this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ }
Review comment:
@virajjasani Good question.
I glimpsed and I guess what @bsglz meant is every time calling the `MoveCostFunction::cost()` function against the same `MoveCostFunction` object should get a different value per the config and the current time. Since this `MoveCostFunction` object is constructed once, the multiplier should be updated here. We can not move the whole if condition to constructor. Updating the `config` object dynamically is another use case (which should also work). Correct me if I'm wrong @bsglz .
That said, to remove `this.conf = conf` dependency, we can get two multipliers from config, and call `setMultiplier()` accordingly here in `cost()` by checking `isOffPeakHour()`.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660592615
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 2m 33s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 20s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 58s | master passed |
| +1 :green_heart: | checkstyle | 2m 16s | master passed |
| +0 :ok: | refguide | 5m 22s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | spotbugs | 11m 45s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 11s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 44s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 18s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 5m 34s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | hadoopcheck | 12m 16s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 12m 3s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 25s | The patch does not generate ASF License warnings. |
| | | 70m 43s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle refguide |
| uname | Linux 1461b6aa7bd3 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-general-check/output/branch-site/book.html |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/artifact/yetus-general-check/output/patch-site/book.html |
| Max. process+thread count | 122 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/5/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660464532
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 19s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 20s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 58s | master passed |
| +1 :green_heart: | checkstyle | 2m 20s | master passed |
| +0 :ok: | refguide | 5m 43s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | spotbugs | 12m 13s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 12s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 49s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 17s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 5m 40s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | hadoopcheck | 12m 47s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 13m 6s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 27s | The patch does not generate ASF License warnings. |
| | | 72m 16s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle refguide |
| uname | Linux 88d3b4a74c8c 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-general-check/output/branch-site/book.html |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-general-check/output/patch-site/book.html |
| Max. process+thread count | 122 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660538716
It's weird, yetus check is consistently failing for JDK11. I don't see suspect code here, triggered one more build. I saw some open master branch PRs, they don't have yetus failures. If next build also fails on yetus check, can we create another PR? I am not very sure but it could be PR specific?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-656863864
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 22s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 56s | master passed |
| +1 :green_heart: | compile | 3m 9s | master passed |
| +1 :green_heart: | shadedjars | 6m 43s | branch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 17s | root in master failed. |
| -0 :warning: | javadoc | 0m 45s | hbase-server in master failed. |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 13s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 40s | the patch passed |
| +1 :green_heart: | compile | 3m 9s | the patch passed |
| +1 :green_heart: | javac | 3m 9s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 33s | patch has no errors when building our shaded downstream artifacts. |
| -0 :warning: | javadoc | 0m 49s | hbase-server in the patch failed. |
| -0 :warning: | javadoc | 0m 15s | root in the patch failed. |
||| _ Other Tests _ |
| +1 :green_heart: | unit | 263m 51s | root in the patch passed. |
| | | 298m 42s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 77fbe74d915b 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 67dfbe0406 |
| Default Java | 2020-01-14 |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-root.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/branch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-hbase-server.txt |
| javadoc | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/artifact/yetus-jdk11-hadoop3-check/output/patch-javadoc-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/testReport/ |
| Max. process+thread count | 4232 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/2/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r452894660
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
Review comment:
Fixed, thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456780861
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY =
+ "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
Review comment:
nit: remove `protected` from both and just keep:
```
static final float DEFAULT_MOVE_COST = 7;
static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660558230
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 22s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 24s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 48s | master passed |
| +1 :green_heart: | checkstyle | 2m 45s | master passed |
| +0 :ok: | refguide | 6m 10s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | spotbugs | 13m 55s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 5s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 30s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 6m 7s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | hadoopcheck | 13m 23s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 14m 41s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 28s | The patch does not generate ASF License warnings. |
| | | 79m 29s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle refguide |
| uname | Linux 1f30b932d48c 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-general-check/output/branch-site/book.html |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-general-check/output/patch-site/book.html |
| Max. process+thread count | 122 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660478646
Thanks @bsglz , let me merge this after QA results are out.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456770295
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java
##########
@@ -284,6 +284,25 @@ public void testLocalityCost() throws Exception {
}
}
+ @Test
+ public void testMoveCostMultiplier() throws Exception {
+ Configuration conf = HBaseConfiguration.create();
+ StochasticLoadBalancer.CostFunction
+ costFunction = new StochasticLoadBalancer.MoveCostFunction(conf);
+ BaseLoadBalancer.Cluster cluster = mockCluster(clusterStateMocks[0]);
+ costFunction.init(cluster);
+ costFunction.cost();
+ assertEquals(7, costFunction.getMultiplier(), 0.01);
Review comment:
Ok, will fix later. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz edited a comment on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz edited a comment on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660481996
Seems the test failture unrelated to this PR, could you help to rerun QA to confirm?
Thanks. @virajjasani
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-657398791
:confetti_ball: **+1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 1m 38s | Docker mode activated. |
||| _ Prechecks _ |
| +1 :green_heart: | dupname | 0m 0s | No case conflicting files found. |
| +1 :green_heart: | hbaseanti | 0m 0s | Patch does not have any anti-patterns. |
| +1 :green_heart: | @author | 0m 0s | The patch does not contain any @author tags. |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 25s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 33s | master passed |
| +1 :green_heart: | checkstyle | 2m 33s | master passed |
| +0 :ok: | refguide | 6m 29s | branch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | spotbugs | 14m 43s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 15s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 29s | the patch passed |
| +1 :green_heart: | checkstyle | 2m 37s | the patch passed |
| +1 :green_heart: | whitespace | 0m 0s | The patch has no whitespace issues. |
| +0 :ok: | refguide | 6m 30s | patch has no errors when building the reference guide. See footer for rendered docs, which you should manually inspect. |
| +1 :green_heart: | hadoopcheck | 14m 21s | Patch does not cause any errors with Hadoop 3.1.2 3.2.1. |
| +1 :green_heart: | spotbugs | 15m 5s | the patch passed |
||| _ Other Tests _ |
| +1 :green_heart: | asflicense | 0m 30s | The patch does not generate ASF License warnings. |
| | | 82m 42s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.9 Server=19.03.9 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-general-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | dupname asflicense spotbugs hadoopcheck hbaseanti checkstyle refguide |
| uname | Linux 023cb3be288b 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / af1cc2fc44 |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-general-check/output/branch-site/book.html |
| refguide | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/artifact/yetus-general-check/output/patch-site/book.html |
| Max. process+thread count | 123 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/1/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) spotbugs=3.1.12 |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] liuml07 commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
liuml07 commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660427619
In my last comment,
> That said, to remove this.conf = conf dependency, we can get two multipliers from config, and call setMultiplier() accordingly here in cost() by checking isOffPeakHour().
I meant in constructor, we can get two multipliers from config and remove the `this.conf` field. In `cost()` we call `setMultiplier()` and choose the right parameter by checking isOffPeakHour().
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456781494
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY =
+ "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
Review comment:
Fixed, thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456767755
##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancer.java
##########
@@ -284,6 +284,25 @@ public void testLocalityCost() throws Exception {
}
}
+ @Test
+ public void testMoveCostMultiplier() throws Exception {
+ Configuration conf = HBaseConfiguration.create();
+ StochasticLoadBalancer.CostFunction
+ costFunction = new StochasticLoadBalancer.MoveCostFunction(conf);
+ BaseLoadBalancer.Cluster cluster = mockCluster(clusterStateMocks[0]);
+ costFunction.init(cluster);
+ costFunction.cost();
+ assertEquals(7, costFunction.getMultiplier(), 0.01);
Review comment:
@liuml07 @bsglz I think it's fine to use constant directly. No worries, keep them public and use it here.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-658504662
@virajjasani Seems no more comments, could you help merge it? Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456751970
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
Review comment:
IMO, the main different of two ways is whether we need to update test code or not when we change the default value.
Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] virajjasani commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
virajjasani commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r456767882
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
private static final int DEFAULT_MAX_MOVES = 600;
private static final float DEFAULT_MAX_MOVE_PERCENT = 0.25f;
private final float maxMovesPercent;
+ private final Configuration conf;
MoveCostFunction(Configuration conf) {
super(conf);
-
- // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
- // that large benefits are need to overcome the cost of a move.
- this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ this.conf = conf;
// What percent of the number of regions a single run of the balancer can move.
maxMovesPercent = conf.getFloat(MAX_MOVES_PERCENT_KEY, DEFAULT_MAX_MOVE_PERCENT);
}
@Override
protected double cost() {
+ // Move cost multiplier should be the same cost or higher than the rest of the costs to ensure
+ // that large benefits are need to overcome the cost of a move.
+ if (OffPeakHours.getInstance(conf).isOffPeakHour()) {
+ this.setMultiplier(conf.getFloat(MOVE_COST_OFFPEAK_KEY, DEFAULT_MOVE_COST_OFFPEAK));
+ } else {
+ this.setMultiplier(conf.getFloat(MOVE_COST_KEY, DEFAULT_MOVE_COST));
+ }
Review comment:
Yeah, this makes sense. +1
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660481996
Seems the test failture unrelated to this PR, could you help to return QA to confirm?
Thanks. @virajjasani
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660586111
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 4m 54s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 23s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 3m 40s | master passed |
| +1 :green_heart: | compile | 2m 22s | master passed |
| +1 :green_heart: | shadedjars | 5m 34s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 40s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 3m 28s | the patch passed |
| +1 :green_heart: | compile | 2m 25s | the patch passed |
| +1 :green_heart: | javac | 2m 25s | the patch passed |
| +1 :green_heart: | shadedjars | 5m 37s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 39s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 344m 22s | root in the patch failed. |
| | | 380m 47s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux fd1d6b275dd3 4.15.0-60-generic #67-Ubuntu SMP Thu Aug 22 16:55:30 UTC 2019 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| Default Java | 1.8.0_232 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/testReport/ |
| Max. process+thread count | 6835 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/4/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
bsglz commented on a change in pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#discussion_r452880162
##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -827,26 +828,34 @@ protected double scale(double min, double max, double value) {
*/
static class MoveCostFunction extends CostFunction {
private static final String MOVE_COST_KEY = "hbase.master.balancer.stochastic.moveCost";
+ private static final String MOVE_COST_OFFPEAK_KEY = "hbase.master.balancer.stochastic.moveCost.offpeak";
private static final String MAX_MOVES_PERCENT_KEY =
"hbase.master.balancer.stochastic.maxMovePercent";
- private static final float DEFAULT_MOVE_COST = 7;
+ @VisibleForTesting
+ protected static final float DEFAULT_MOVE_COST = 7;
+ protected static final float DEFAULT_MOVE_COST_OFFPEAK = 3;
Review comment:
Good advice, will fix.
Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [hbase] Apache-HBase commented on pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…
Posted by GitBox <gi...@apache.org>.
Apache-HBase commented on pull request #2044:
URL: https://github.com/apache/hbase/pull/2044#issuecomment-660551675
:broken_heart: **-1 overall**
| Vote | Subsystem | Runtime | Comment |
|:----:|----------:|--------:|:--------|
| +0 :ok: | reexec | 0m 33s | Docker mode activated. |
| -0 :warning: | yetus | 0m 3s | Unprocessed flag(s): --brief-report-file --spotbugs-strict-precheck --whitespace-eol-ignore-list --whitespace-tabs-ignore-list --quick-hadoopcheck |
||| _ Prechecks _ |
||| _ master Compile Tests _ |
| +0 :ok: | mvndep | 0m 22s | Maven dependency ordering for branch |
| +1 :green_heart: | mvninstall | 4m 4s | master passed |
| +1 :green_heart: | compile | 2m 33s | master passed |
| +1 :green_heart: | shadedjars | 6m 10s | branch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 3m 25s | master passed |
||| _ Patch Compile Tests _ |
| +0 :ok: | mvndep | 0m 14s | Maven dependency ordering for patch |
| +1 :green_heart: | mvninstall | 4m 25s | the patch passed |
| +1 :green_heart: | compile | 2m 58s | the patch passed |
| +1 :green_heart: | javac | 2m 58s | the patch passed |
| +1 :green_heart: | shadedjars | 6m 15s | patch has no errors when building our shaded downstream artifacts. |
| +1 :green_heart: | javadoc | 2m 51s | the patch passed |
||| _ Other Tests _ |
| -1 :x: | unit | 395m 29s | root in the patch failed. |
| | | 432m 0s | |
| Subsystem | Report/Notes |
|----------:|:-------------|
| Docker | Client=19.03.12 Server=19.03.12 base: https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk8-hadoop3-check/output/Dockerfile |
| GITHUB PR | https://github.com/apache/hbase/pull/2044 |
| Optional Tests | javac javadoc unit shadedjars compile |
| uname | Linux 1a94f71c1aae 4.15.0-101-generic #102-Ubuntu SMP Mon May 11 10:07:26 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux |
| Build tool | maven |
| Personality | dev-support/hbase-personality.sh |
| git revision | master / 9b02a26a1d |
| Default Java | 1.8.0_232 |
| unit | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/artifact/yetus-jdk8-hadoop3-check/output/patch-unit-root.txt |
| Test Results | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/testReport/ |
| Max. process+thread count | 4128 (vs. ulimit of 12500) |
| modules | C: hbase-server . U: . |
| Console output | https://builds.apache.org/job/HBase-PreCommit-GitHub-PR/job/PR-2044/3/console |
| versions | git=2.17.1 maven=(cecedd343002696d0abb50b32b541b8a6ba2883f) |
| Powered by | Apache Yetus 0.11.1 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org