ndimiduk commented on code in PR #6597:
URL: https://github.com/apache/hbase/pull/6597#discussion_r1913151169
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/StochasticBalancerTestBase.java:
##########
@@ -48,17 +49,15 @@ public static void beforeAllTests() throws Exception {
conf.setClass("hbase.util.ip.to.rack.determiner", MockMapping.class,
DNSToSwitchMapping.class);
conf.setFloat("hbase.master.balancer.stochastic.localityCost", 0);
conf.setBoolean("hbase.master.balancer.stochastic.runMaxSteps", true);
+ conf.setLong(StochasticLoadBalancer.MAX_RUNNING_TIME_KEY, 250);
Review Comment:
Why override with such a small value?
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CostFunction.java:
##########
@@ -25,7 +25,9 @@
@InterfaceAudience.Private
abstract class CostFunction {
- public static final double COST_EPSILON = 0.0001;
+ public static double getCostEpsilon(double cost) {
+ return Math.ulp(cost);
Review Comment:
TIL
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/StochasticBalancerTestBase.java:
##########
@@ -114,7 +85,13 @@ protected void testWithClusterWithIteration(Map<ServerName,
List<RegionInfo>> se
Map<TableName, Map<ServerName, List<RegionInfo>>> LoadOfAllTable =
(Map) mockClusterServersWithTables(serverMap);
List<RegionPlan> plans = loadBalancer.balanceCluster(LoadOfAllTable);
- assertNotNull("Initial cluster balance should produce plans.", plans);
+ if (plans == null) {
+ LOG.info("First plans are null. Trying more balancer time, or will
fail");
Review Comment:
I think that these log messages about relaxing the runtime constraint should
be `DEBUG`.
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/TestStochasticLoadBalancerHeterogeneousCost.java:
##########
@@ -306,6 +245,10 @@ static class StochasticLoadTestBalancer extends
StochasticLoadBalancer {
private FairRandomCandidateGenerator fairRandomCandidateGenerator =
new FairRandomCandidateGenerator();
+ StochasticLoadTestBalancer() {
+ super(new DummyMetricsStochasticBalancer());
+ }
Review Comment:
Do we see the impact of this bug in our CI runs?
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/StochasticBalancerTestBase.java:
##########
@@ -36,6 +36,7 @@
public class StochasticBalancerTestBase extends BalancerTestBase {
private static final Logger LOG =
LoggerFactory.getLogger(StochasticBalancerTestBase.class);
+ private static final long MAX_MAX_RUN_TIME_MS = 60000;
Review Comment:
nit: I prefer that we use values computed from the fluent API on `TimeUnit`
rather than literals that assume units.
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/StochasticBalancerTestBase.java:
##########
@@ -48,17 +49,15 @@ public static void beforeAllTests() throws Exception {
conf.setClass("hbase.util.ip.to.rack.determiner", MockMapping.class,
DNSToSwitchMapping.class);
conf.setFloat("hbase.master.balancer.stochastic.localityCost", 0);
conf.setBoolean("hbase.master.balancer.stochastic.runMaxSteps", true);
+ conf.setLong(StochasticLoadBalancer.MAX_RUNNING_TIME_KEY, 250);
loadBalancer = new StochasticLoadBalancer(dummyMetricsStochasticBalancer);
loadBalancer.setClusterInfoProvider(new DummyClusterInfoProvider(conf));
loadBalancer.initialize();
}
- protected void testWithCluster(int numNodes, int numRegions, int
numRegionsPerServer,
- int replication, int numTables, boolean assertFullyBalanced,
- boolean assertFullyBalancedForReplicas) {
- Map<ServerName, List<RegionInfo>> serverMap =
- createServerMap(numNodes, numRegions, numRegionsPerServer, replication,
numTables);
- testWithCluster(serverMap, null, assertFullyBalanced,
assertFullyBalancedForReplicas);
+ protected void setMaxRunTime(long maxRunTimeMs) {
+ conf.setLong(StochasticLoadBalancer.MAX_RUNNING_TIME_KEY, maxRunTimeMs);
+ loadBalancer.loadConf(conf);
}
protected void testWithClusterWithIteration(int numNodes, int numRegions,
int numRegionsPerServer,
Review Comment:
I know it's not your code, but this argument list is long enough and has
enough overlapping argument types that I think a build object is warranted. It
would be nice if we could introduce something like http://immutables.github.io/
but that's not this PR.
##########
hbase-balancer/src/test/java/org/apache/hadoop/hbase/master/balancer/StochasticBalancerTestBase.java:
##########
@@ -48,17 +49,15 @@ public static void beforeAllTests() throws Exception {
conf.setClass("hbase.util.ip.to.rack.determiner", MockMapping.class,
DNSToSwitchMapping.class);
conf.setFloat("hbase.master.balancer.stochastic.localityCost", 0);
conf.setBoolean("hbase.master.balancer.stochastic.runMaxSteps", true);
+ conf.setLong(StochasticLoadBalancer.MAX_RUNNING_TIME_KEY, 250);
loadBalancer = new StochasticLoadBalancer(dummyMetricsStochasticBalancer);
loadBalancer.setClusterInfoProvider(new DummyClusterInfoProvider(conf));
loadBalancer.initialize();
}
- protected void testWithCluster(int numNodes, int numRegions, int
numRegionsPerServer,
- int replication, int numTables, boolean assertFullyBalanced,
- boolean assertFullyBalancedForReplicas) {
- Map<ServerName, List<RegionInfo>> serverMap =
- createServerMap(numNodes, numRegions, numRegionsPerServer, replication,
numTables);
- testWithCluster(serverMap, null, assertFullyBalanced,
assertFullyBalancedForReplicas);
+ protected void setMaxRunTime(long maxRunTimeMs) {
Review Comment:
nit: likewise, this method signature could be more ergonomic, like the
modern timeout methods in the JDK. Something like `setMaxRunTime(long timeout,
TimeUnit unit)` makes it more difficult to misinterpret the value between
caller and callee.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
