[GitHub] [hbase] saintstack commented on a change in pull request #3248: HBASE-25852 Move all the intialization work of LoadBalancer implement…

[GitBox <gi...@apache.org>]

saintstack commented on a change in pull request #3248:
URL: https://github.com/apache/hbase/pull/3248#discussion_r629719036



##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/LoadBalancer.java
##########
@@ -59,10 +59,11 @@
    */
   @Deprecated
   String HBASE_RSGROUP_LOADBALANCER_CLASS = "hbase.rsgroup.grouploadbalancer.class";
+
   /**
    * Set the current cluster status. This allows a LoadBalancer to map host name to a server
    */
-  void setClusterMetrics(ClusterMetrics st);
+  void updateClusterMetrics(ClusterMetrics metrics);

Review comment:
       We change the method name because the method is now being used differently? Before we called 'set' once? But now we 'update' continuously during operation? Or was it just always incorrectly named?

##########
File path: hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
##########
@@ -53,6 +52,10 @@
  * The base class for load balancers. It provides the the functions used to by
  * {@code AssignmentManager} to assign regions in the edge cases. It doesn't provide an
  * implementation of the actual balancing algorithm.
+ * <p/>
+ * Since 3.0.0, all the balancers will be wrapped inside a {@code RSGroupBasedLoadBalancer}, it will
+ * be in charge of the synchronization of balancing and configuration changing, so we do not need to
+ * synchronized by ourselves.

Review comment:
       Interesting. Wondering what the rationale for this (Perhaps it later in this patch)

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/balancer/StochasticLoadBalancer.java
##########
@@ -400,6 +387,12 @@ BalanceAction nextAction(BalancerClusterState cluster) {
       .generate(cluster);
   }
 
+  @RestrictedApi(explanation = "Should only be called in tests", link = "",

Review comment:
       Instead of @VisibleForTests? (I like this annotation)




-- 
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