rmdmattingly commented on code in PR #6763:
URL: https://github.com/apache/hbase/pull/6763#discussion_r1987049620
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java:
##########
@@ -53,6 +55,12 @@
public class CacheAwareLoadBalancer extends StochasticLoadBalancer {
private static final Logger LOG =
LoggerFactory.getLogger(CacheAwareLoadBalancer.class);
+ public static final String CACHE_RATIO_THRESHOLD =
"hbase.master.balancer.stochastic.cacheRatio";
+
+ public static final String MOVE_THROTTLING =
"hbase.master.balancer.move.throttlingMs";
Review Comment:
If throttle support is only added to the cache aware balancer, then maybe
this configuration name should also be specific to the cache aware balancer
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java:
##########
@@ -53,6 +55,12 @@
public class CacheAwareLoadBalancer extends StochasticLoadBalancer {
private static final Logger LOG =
LoggerFactory.getLogger(CacheAwareLoadBalancer.class);
+ public static final String CACHE_RATIO_THRESHOLD =
"hbase.master.balancer.stochastic.cacheRatio";
Review Comment:
If this ratio is only used for throttling, then I wonder whether we should
name it more specifically
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java:
##########
@@ -160,6 +168,56 @@ private RegionInfo
getRegionInfoByEncodedName(BalancerClusterState cluster, Stri
return null;
}
+ @Override
+ public void throttle(RegionPlan plan) {
+ Pair<ServerName, Float> rsRatio =
this.regionCacheRatioOnOldServerMap.get(plan.getRegionName());
+ float ratioThreshold =
+ this.configuration.getFloat(CACHE_RATIO_THRESHOLD,
CACHE_RATIO_THRESHOLD_DEFAULT);
Review Comment:
This could be a pretty hot path on a bigger, imbalanced cluster. So I wonder
if we should move this conf lookup to somewhere less hot?
##########
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/CacheAwareLoadBalancer.java:
##########
@@ -53,6 +55,12 @@
public class CacheAwareLoadBalancer extends StochasticLoadBalancer {
private static final Logger LOG =
LoggerFactory.getLogger(CacheAwareLoadBalancer.class);
+ public static final String CACHE_RATIO_THRESHOLD =
"hbase.master.balancer.stochastic.cacheRatio";
+
+ public static final String MOVE_THROTTLING =
"hbase.master.balancer.move.throttlingMs";
+
+ public static final long MOVE_THROTTLING_DEFAULT = 60 * 1000;
Review Comment:
nit: the unit is ambiguous. We should either use a more intentional type
like Duration, or we should include the unit in the name
--
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]
