[GitHub] [hbase] saintstack commented on a change in pull request #1613: HBASE-24273 HBCK's "Orphan Regions on FileSystem" reports regions wit…

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

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



##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HbckChore.java
##########
@@ -134,7 +135,7 @@ protected synchronized void chore() {
       loadRegionsFromInMemoryState();
       loadRegionsFromRSReport();
       try {
-        loadRegionsFromFS();
+        loadRegionsFromFS(scanForMergedParentRegions());

Review comment:
       Does InMemoryState not have the needed merge info in it? If not, maybe it should.
   
   The CatalogJanitor is what manages when merge references are let go so this scan of meta is probably necessary.
   
   To the @timoha point, are there other places in hbckchore where we need currrent picture of hbase:meta?

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HbckChore.java
##########
@@ -271,12 +297,12 @@ private void loadRegionsFromFS() throws IOException {
           continue;
         }
         HbckRegionInfo hri = regionInfoMap.get(encodedRegionName);
-        if (hri == null) {
+        // If it is not in in-memory database and not a merged region,
+        // report it as an orphan region.
+        if (hri == null && !mergedParentRegions.contains(encodedRegionName)) {

Review comment:
       Oh, merge parents are NOT in in-memory state because they are not active. We just have this background cleaner task that is doing janitorial work on hbase:meta cleaning up meta and filesystem....

##########
File path: hbase-server/src/test/java/org/apache/hadoop/hbase/client/HConnectionTestingUtility.java
##########
@@ -39,12 +39,17 @@
    * probably not what you want.
    * @param conf configuration
    * @return ConnectionImplementation object for <code>conf</code>
-   * @throws ZooKeeperConnectionException
+   * @throws IOException

Review comment:
       Empty javadoc like this provokes complaint by checkstyle. Just remove it.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/HbckChore.java
##########
@@ -187,6 +188,31 @@ private void saveCheckResultToSnapshot() {
     }
   }
 
+  /**
+   * Scan hbase:meta to get set of merged parent regions, this is a very heavy scan.
+   *
+   * @return Return generated {@link HashSet}
+   */
+  private HashSet<String> scanForMergedParentRegions() throws IOException {
+    HashSet<String> mergedParentRegions = new HashSet<>();
+    // Null tablename means scan all of meta.
+    MetaTableAccessor.scanMetaForTableRegions(this.master.getConnection(),
+      r -> {
+        List<RegionInfo> mergeParents = MetaTableAccessor.getMergeRegions(r.rawCells());
+        if (mergeParents != null) {
+          for (RegionInfo mergeRegion : mergeParents) {
+            if (mergeRegion != null) {
+              // This region is already being merged
+              mergedParentRegions.add(mergeRegion.getEncodedName());
+            }
+          }
+        }
+        return true;
+        },
+      null);
+    return mergedParentRegions;
+  }
+

Review comment:
       Yeah, may be no way around it given the scan for merged parents is so specialized.
   
   This looks good.

##########
File path: hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java
##########
@@ -1859,11 +1859,17 @@ public void markRegionAsMerged(final RegionInfo child, final ServerName serverNa
       throws IOException {
     final RegionStateNode node = regionStates.getOrCreateRegionStateNode(child);
     node.setState(State.MERGED);
+    regionStateStore.mergeRegions(child, mergeParents, serverName);
+
+    // The order of adding merge qualifers and deleting from regionStates is important.
+    // hbck chore depends on these merge qualifers and regionStates to check if a region on FS
+    // is an orphan. If deleting from regionStates first, then there is a small gap that
+    // a region is not in regionStates and there is no merge qualifer for this region, it will be
+    // reported as an orphan.
     for (RegionInfo ri: mergeParents) {
       regionStates.deleteRegion(ri);
 
     }
-    regionStateStore.mergeRegions(child, mergeParents, serverName);

Review comment:
       ok. good. lets keep an eye on this one because sometimes fun, unexpected conditions when change order of operations.




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