subject:"\[GitHub\] \[hbase\] bsglz commented on a change in pull request #2044\: HBASE\-24709 Support MoveCostFunction use a lower multiplier in offpea…"

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-18 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-18 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-17 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-17 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-17 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-17 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-10 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-10 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

2020-07-10 Thread GitBox



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:
us...@infra.apache.org

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

[GitHub] [hbase] bsglz commented on a change in pull request #2044: HBASE-24709 Support MoveCostFunction use a lower multiplier in offpea…

9 matches

Site Navigation

Mail list logo

Footer information