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