nirdosh0110 commented on code in PR #8599:
URL: https://github.com/apache/hbase/pull/8599#discussion_r4000393841
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java:
##########
@@ -294,10 +303,79 @@ static void removeNonDefaultReplicas(MasterProcedureEnv
env, Stream<RegionInfo>
}
static void checkClosedRegion(MasterProcedureEnv env, RegionInfo regionInfo)
throws IOException {
- if (WALSplitUtil.hasRecoveredEdits(env.getMasterConfiguration(),
regionInfo)) {
- throw new IOException("Recovered.edits are found in Region: " +
regionInfo
- + ", abort split/merge to prevent data loss");
+ if (!WALSplitUtil.hasRecoveredEdits(env.getMasterConfiguration(),
regionInfo)) {
+ return;
}
+ // A recovered.edits file whose max seqid is <= the region's last flushed
seqid is stale:
Review Comment:
Done — reframed as a generic robustness note without the incident-specific
detail in c7e4623.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java:
##########
@@ -294,10 +303,79 @@ static void removeNonDefaultReplicas(MasterProcedureEnv
env, Stream<RegionInfo>
}
static void checkClosedRegion(MasterProcedureEnv env, RegionInfo regionInfo)
throws IOException {
- if (WALSplitUtil.hasRecoveredEdits(env.getMasterConfiguration(),
regionInfo)) {
- throw new IOException("Recovered.edits are found in Region: " +
regionInfo
- + ", abort split/merge to prevent data loss");
+ if (!WALSplitUtil.hasRecoveredEdits(env.getMasterConfiguration(),
regionInfo)) {
+ return;
}
+ // A recovered.edits file whose max seqid is <= the region's last flushed
seqid is stale:
+ // its edits are already durable in HFiles. This happens e.g. when a
graceful region move
+ // is followed by a WAL split of the source RS - the split creates
recovered.edits for a
+ // region that has already been reopened elsewhere and flushed. Cleaning
up such files here
+ // (rather than aborting split/merge) matches the tolerance HRegion itself
applies at open.
+ if (tryDropStaleRecoveredEdits(env, regionInfo)) {
+ return;
+ }
+ throw new IOException("Recovered.edits are found in Region: " + regionInfo
+ + ", abort split/merge to prevent data loss");
+ }
+
+ /**
+ * Try to remove recovered.edits files that are provably below the region's
last flushed seqid.
+ * @return true if, after cleanup, no recovered.edits remain for the region
+ */
+ private static boolean tryDropStaleRecoveredEdits(MasterProcedureEnv env,
RegionInfo regionInfo) {
+ long durableSeqId = env.getMasterServices().getServerManager()
+
.getLastFlushedSequenceId(regionInfo.getEncodedNameAsBytes()).getLastFlushedSequenceId();
+ if (durableSeqId <= 0L) {
Review Comment:
Good point. The reason for going with `ServerManager` here was consistency —
SCP consults the same map when deciding what enters `recovered.edits` in the
first place, so both decisions come from one source and cannot disagree. That
said, HFile metadata is stronger: it is the on-disk truth and would decouple
this check from HBASE-30335's seed timing (and from a cold `ServerManager`
cache right after master restart).
Reading it correctly means `min across families of max HFile seqid` (the
same fence `HRegion.replayRecoveredEditsIfAny` uses when skipping
already-flushed edits). Roughly: `HRegionFileSystem.openRegionFromFileSystem` →
per family `StoreFileTracker.load()` → wrap each `StoreFileInfo` in
`HStoreFile` with `CacheConfig.DISABLED` → `getReader().getSequenceID()`. Same
pattern `MergeTableRegionsProcedure.mergeStoreFiles` already uses in this
package.
Would you prefer I switch entirely to the HFile-metadata source in this PR,
or keep `ServerManager` primary with HFile-metadata as a fallback when the
cache is empty? Happy to do either.
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java:
##########
@@ -294,10 +303,79 @@ static void removeNonDefaultReplicas(MasterProcedureEnv
env, Stream<RegionInfo>
}
static void checkClosedRegion(MasterProcedureEnv env, RegionInfo regionInfo)
throws IOException {
- if (WALSplitUtil.hasRecoveredEdits(env.getMasterConfiguration(),
regionInfo)) {
- throw new IOException("Recovered.edits are found in Region: " +
regionInfo
- + ", abort split/merge to prevent data loss");
+ if (!WALSplitUtil.hasRecoveredEdits(env.getMasterConfiguration(),
regionInfo)) {
+ return;
}
+ // A recovered.edits file whose max seqid is <= the region's last flushed
seqid is stale:
+ // its edits are already durable in HFiles. This happens e.g. when a
graceful region move
+ // is followed by a WAL split of the source RS - the split creates
recovered.edits for a
+ // region that has already been reopened elsewhere and flushed. Cleaning
up such files here
+ // (rather than aborting split/merge) matches the tolerance HRegion itself
applies at open.
+ if (tryDropStaleRecoveredEdits(env, regionInfo)) {
+ return;
+ }
+ throw new IOException("Recovered.edits are found in Region: " + regionInfo
+ + ", abort split/merge to prevent data loss");
+ }
+
+ /**
+ * Try to remove recovered.edits files that are provably below the region's
last flushed seqid.
+ * @return true if, after cleanup, no recovered.edits remain for the region
+ */
+ private static boolean tryDropStaleRecoveredEdits(MasterProcedureEnv env,
RegionInfo regionInfo) {
+ long durableSeqId = env.getMasterServices().getServerManager()
+
.getLastFlushedSequenceId(regionInfo.getEncodedNameAsBytes()).getLastFlushedSequenceId();
+ if (durableSeqId <= 0L) {
+ // No authoritative durability info at the master; play safe and let the
caller abort.
+ return false;
+ }
+ try {
+ Configuration conf = env.getMasterConfiguration();
+ Path regionWALDir =
+ CommonFSUtils.getWALRegionDir(conf, regionInfo.getTable(),
regionInfo.getEncodedName());
+ Path regionDir =
FSUtils.getRegionDirFromRootDir(CommonFSUtils.getRootDir(conf), regionInfo);
+ Path wrongRegionWALDir = CommonFSUtils.getWrongWALRegionDir(conf,
regionInfo.getTable(),
+ regionInfo.getEncodedName());
+ FileSystem walFs = CommonFSUtils.getWALFileSystem(conf);
+ FileSystem rootFs = CommonFSUtils.getRootDirFileSystem(conf);
+ return dropStaleEditsUnder(walFs, regionWALDir, durableSeqId, regionInfo)
+ && dropStaleEditsUnder(rootFs, regionDir, durableSeqId, regionInfo)
+ && dropStaleEditsUnder(walFs, wrongRegionWALDir, durableSeqId,
regionInfo);
+ } catch (IOException e) {
+ LOG.warn("Failed to inspect recovered.edits for {}; falling back to
abort", regionInfo, e);
+ return false;
+ }
+ }
+
+ private static boolean dropStaleEditsUnder(FileSystem fs, Path regionDir,
long durableSeqId,
+ RegionInfo regionInfo) throws IOException {
+ NavigableSet<Path> files = WALSplitUtil.getSplitEditFilesSorted(fs,
regionDir);
+ if (files.isEmpty()) {
+ return true;
+ }
+ for (Path p : files) {
+ long fileMaxSeqId;
+ try {
+ fileMaxSeqId = Long.parseLong(p.getName());
+ } catch (NumberFormatException e) {
+ LOG.warn("Non-numeric recovered.edits filename {} for {}; not
dropping", p, regionInfo);
Review Comment:
You're right — `WALSplitUtil.getSplitEditFilesSorted` already restricts
filenames via `EDITFILES_NAME_PATTERN` (`-?[0-9]+`), so `Long.parseLong` cannot
throw here. Removed the guard in c7e4623 and left a short note pointing at the
pattern.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
