[GitHub] [hbase] bharathv commented on a change in pull request #3219: HBASE-25836 RegionStates#getAssignmentsForBalancer should only care about OPEN or OPENING regions

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

bharathv commented on a change in pull request #3219:
URL: https://github.com/apache/hbase/pull/3219#discussion_r624900154



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java
##########
@@ -571,23 +570,25 @@ public ServerName getRegionServerOfRegion(RegionInfo regionInfo) {
    * wants to iterate this exported list.  We need to synchronize on regions
    * since all access to this.servers is under a lock on this.regions.
    *
-   * @return A clone of current assignments.
+   * @return A clone of current open or opening assignments.
    */
   public Map<TableName, Map<ServerName, List<RegionInfo>>> getAssignmentsForBalancer(
       TableStateManager tableStateManager, List<ServerName> onlineServers) {
     final Map<TableName, Map<ServerName, List<RegionInfo>>> result = new HashMap<>();
     for (RegionStateNode node : regionsMap.values()) {
-      if (isTableDisabled(tableStateManager, node.getTable())) {

Review comment:
       nit: DisableTableProcedure first sets the table state to DISABLED and then force unassigns the regions in a loop, so we may want to retain this check to avoid weird races when "disable table" and the "chore" is running to totally avoid the table when it is marked DISABLED but regions are still a WIP.
   

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/RegionStates.java
##########
@@ -571,23 +570,25 @@ public ServerName getRegionServerOfRegion(RegionInfo regionInfo) {
    * wants to iterate this exported list.  We need to synchronize on regions
    * since all access to this.servers is under a lock on this.regions.
    *
-   * @return A clone of current assignments.
+   * @return A clone of current open or opening assignments.
    */
   public Map<TableName, Map<ServerName, List<RegionInfo>>> getAssignmentsForBalancer(
       TableStateManager tableStateManager, List<ServerName> onlineServers) {
     final Map<TableName, Map<ServerName, List<RegionInfo>>> result = new HashMap<>();
     for (RegionStateNode node : regionsMap.values()) {
-      if (isTableDisabled(tableStateManager, node.getTable())) {
-        continue;
-      }
-      if (node.getRegionInfo().isSplitParent()) {
+      // When balancing, we are only interested in OPEN or OPENING regions and expected
+      // to be online at that server until possibly the next balancer iteration or unless
+      // we decide to move it. Other states are not interesting as the region will either
+      // be closing, or splitting/merging, or will not be deployed.
+      if (!(node.isInState(State.OPEN)||node.isInState(State.OPENING))) {

Review comment:
       nit: spaces missing on either side of ||




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