HBASE-30352: Tolerate stale recovered.edits below durable seqid in split/merge - #8599
nirdosh0110 wants to merge 2 commits into
Conversation
…it/merge MergeTableRegionsProcedure and SplitTableRegionProcedure invoke AssignmentManagerUtil.checkClosedRegion during MERGE/SPLIT_CHECK_CLOSED_REGIONS. Today the check aborts the procedure if any recovered.edits file exists in the region directory, retaining the region lock and leaving the region CLOSED/RIT until the next master failover. This can happen on a graceful region move followed by the source RS's WAL split completing after the region has already reopened - and flushed - on the target RS. The recovered.edits file contains edits already durable in HFiles; the existence of the file is harmless but the check treats it as data-loss risk. Change checkClosedRegion so that, when recovered.edits are present, it consults ServerManager.getLastFlushedSequenceId for the region and inspects each recovered.edits filename (whose numeric name is the max seqid of edits in the file). If every file's max seqid is <= the region's durable seqid the files are removed and the procedure proceeds. Otherwise the previous abort behavior is preserved as a safe fallback. Follow-up to the discussion on PR apache#8584 (HBASE-30335).
nirdosh0110
changed the title
nirdosh0110
changed the title
|
@Apache9 @apurtell @virajjasani Please help with review on this PR. TIA. |
|
@nirdosh0110 I just realized this PR also points to HBASE-30335, we will need new Jira for this PR, given the 1:1 mapping that we follow for each Jira/PR. |
nirdosh0110
changed the title
Thanks @virajjasani. Some how this PR is not auto linked to open jira. Below are the details. I've updated the PR title. |
Now it is linked properly. |
| if (!WALSplitUtil.hasRecoveredEdits(env.getMasterConfiguration(), regionInfo)) { | ||
| return; | ||
| } | ||
| // A recovered.edits file whose max seqid is <= the region's last flushed seqid is stale: |
There was a problem hiding this comment.
I prefer we do not mention the details about how we can generate stale recoverd.edits, as in general, we should fix the problem.
Just add comment to say that this is for making the process more robust, there may be some corner cases where we left stale recovered.edits.
There was a problem hiding this comment.
Done — reframed as a generic robustness note without the incident-specific detail in c7e4623.
| private static boolean tryDropStaleRecoveredEdits(MasterProcedureEnv env, RegionInfo regionInfo) { | ||
| long durableSeqId = env.getMasterServices().getServerManager() | ||
| .getLastFlushedSequenceId(regionInfo.getEncodedNameAsBytes()).getLastFlushedSequenceId(); | ||
| if (durableSeqId <= 0L) { |
There was a problem hiding this comment.
We can get this value by reading all the HFiles' metadata?
There was a problem hiding this 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.
| try { | ||
| fileMaxSeqId = Long.parseLong(p.getName()); | ||
| } catch (NumberFormatException e) { | ||
| LOG.warn("Non-numeric recovered.edits filename {} for {}; not dropping", p, regionInfo); |
There was a problem hiding this comment.
Does this really hurt? When loading recovered.edits, we will abort or just skip these files?
There was a problem hiding this 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.
- Replace incident-specific block comment in checkClosedRegion with a generic "corner-case robustness" note. - Remove the NumberFormatException guard in dropStaleEditsUnder: filenames returned by WALSplitUtil.getSplitEditFilesSorted are constrained by EDITFILES_NAME_PATTERN (`-?[0-9]+`), so Long.parseLong cannot throw here.
3 participants
Context
Follow-up to the discussion on #8584 (HBASE-30335). @Apache9 suggested in #8584 (comment):
This PR implements that check for both
MergeTableRegionsProcedureandSplitTableRegionProcedure(both callAssignmentManagerUtil.checkClosedRegion).JIRA: https://issues.apache.org/jira/browse/HBASE-30352
The incident this addresses
112d9f08was gracefully moved from RS-A → RS-B.openSeqNum=4997750282; all prior edits were durable in HFiles.recovered.editsfile for this region containingseqId=4997750280— an edit already flushed on HFile before RS-A closed.MergeTableRegionsProcedurelater hitMERGE_TABLE_REGIONS_CHECK_CLOSED_REGIONS, saw therecovered.editsfile, and threw. The region sat in CLOSED/RIT for ~49 min until master failover cleared it.Change
AssignmentManagerUtil.checkClosedRegionnow, whenhasRecoveredEditsis true:lastFlushedSequenceIdfromServerManager.formatRecoveredEditsFileName(maxEditWALSeqNum)— to get the file's max seqid.lastFlushedSequenceId, deletes those specific files and returns.Filename parsing avoids opening/reading the WAL edits; the writer contract already encodes the max seqid in the file name via
WALSplitUtil.getCompletedRecoveredEditsFilePath.Files
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManagerUtil.java— the tolerance logic.hbase-server/src/test/java/org/apache/hadoop/hbase/master/assignment/TestAssignmentManagerUtil.java— new testtestCheckClosedRegionDropsStaleRecoveredEditsverifies (a) a stale file is dropped and the check passes, (b) a file with a fresh seqid still causes the abort.Local run
Notes / open questions
ServerManagerhas an authoritativelastFlushedSequenceIdfor the region. With HBASE-30335 landing, this will be the case immediately after region OPEN. Without HBASE-30335 it kicks in after the first flush heartbeat. In either case, the fallback matches today's behavior.recovered.editson region OPEN. Happy to file/pick that up separately if reviewers agree it should be in scope.