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:
[email protected]
