Apache9 commented on a change in pull request #2569:
URL: https://github.com/apache/hbase/pull/2569#discussion_r508577459



##########
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/master/procedure/DeleteTableProcedure.java
##########
@@ -100,6 +101,12 @@ protected Flow executeFromState(final MasterProcedureEnv 
env, DeleteTableState s
           // TODO: Move out... in the acquireLock()
           LOG.debug("Waiting for RIT for {}", this);
           regions = 
env.getAssignmentManager().getRegionStates().getRegionsOfTable(getTableName());
+          // We need to archive SPLIT regions as well, so add the SPLIT 
regions to the region list
+          List<RegionInfo> splitRegions = 
env.getAssignmentManager().getRegionStates()

Review comment:
       I think here we could just get all the regions of the table.
   
   And look at the code of getRegionsOfTable(TableName, boolean), I'm afraid 
the implementation of the include method has something wrong?
   
   ```
       if (node.isInState(State.SPLIT) || hri.isSplit()) {
         return false;
       }
       if ((node.isInState(State.OFFLINE) || hri.isOffline()) && !offline) {
         return false;
       }
       return (!hri.isOffline() && !hri.isSplit()) ||
           ((hri.isOffline() || hri.isSplit()) && offline);
   ```
   
   I do not think we need the two extra if conditions? Especially the first 
one, it will make us always exclude split parent?




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


Reply via email to