Apache9 commented on a change in pull request #3217:
URL: https://github.com/apache/hbase/pull/3217#discussion_r625141571
##########
File path:
hbase-balancer/src/main/java/org/apache/hadoop/hbase/master/balancer/BaseLoadBalancer.java
##########
@@ -610,13 +610,35 @@ private void roundRobinAssignment(BalancerClusterState
cluster, List<RegionInfo>
return returnMap;
}
- @Override
- public abstract List<RegionPlan> balanceTable(TableName tableName,
- Map<ServerName, List<RegionInfo>> loadOfOneTable);
+ /**
+ * Perform the major balance operation for table, all class implement of
{@link LoadBalancer}
+ * should override this method
+ * @param tableName the table to be balanced
+ * @param loadOfOneTable region load of servers for the specific one table
+ * @return List of plans
+ */
+ protected abstract List<RegionPlan> balanceTable(TableName tableName,
+ Map<ServerName, List<RegionInfo>> loadOfOneTable);
+ /**
+ * Called before actually executing balanceCluster. The sub classes could
override this method to
+ * do some initialization work.
+ */
+ protected void
+ preBalanceCluster(Map<TableName, Map<ServerName, List<RegionInfo>>>
loadOfAllTable) {
+ }
+
+ /**
+ * Perform the major balance operation for cluster, will invoke {@link
#balanceTable} to do actual
+ * balance.
+ * @param loadOfAllTable region load of servers for all table
+ * @return a list of regions to be moved, including source and destination,
or null if cluster is
+ * already balanced
+ */
@Override
- public List<RegionPlan>
- balanceCluster(Map<TableName, Map<ServerName, List<RegionInfo>>>
loadOfAllTable) {
+ public final synchronized List<RegionPlan>
+ balanceCluster(Map<TableName, Map<ServerName, List<RegionInfo>>>
loadOfAllTable) {
Review comment:
Glad to see you back!
In the past we allow SimpleLoadBalancer and RSGroupBasedLoadBalancer to
override this method, but now, RSGroupBasedLoadBalancer just implements
LoadBalancer interface, and in this patch, I added a preBalanceCluster method
so SimpleLoadBalancer does not need to override this method too, then I marked
this method as final.
For me, I think we should mark a method final if possible, to let people
understand the code easier. This is called BaseLoadBalancer, and we have a
implementation of balanceCluster in this class. It will be a bit confusing that
a sub class will complete rewrite this method. If you do so, then maybe it
should not extend BaseLoadBalancer?
--
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
