HBASE-30335 Seed master flushedSequenceIdByRegion with openSeqNum on region OPEN by nirdosh0110 · Pull Request #8584 · apache/hbase

nirdosh0110 · 2026-08-28T09:41:21Z

Summary

The master's flushedSequenceIdByRegion is only updated via periodic RegionServer heartbeats. On a fresh region OPEN, the entry is absent, so ServerManager.getLastFlushedSequenceId returns NO_SEQNUM and WALSplitter treats every WAL edit as un-flushed. If the source RS of a drain-move crashes before its next heartbeat, this produces orphaned recovered.edits files containing already-durable edits, which then surface as false-positive "data loss" warnings during subsequent merge/split operations and leave regions stuck in RIT.
Add ServerManager.reportRegionOpen(regionInfo, openSeqNum) and call it from AssignmentManager.regionOpenedWithoutPersistingToMeta so the master seeds its flushed-sequence cache synchronously with the openSeqNum reported on the OPEN transition.
putIfAbsent is used so a heartbeat-supplied value (which may reflect flushes after open) is never regressed.

JIRA: https://issues.apache.org/jira/browse/HBASE-30335

Test plan

Existing TestAssignmentManager and TestServerManager suites pass.
Reproduce: drain-move a region, kill source RS before next heartbeat, verify no recovered.edits are produced for already-flushed sequence numbers and no downstream stuck RIT on a follow-up merge.

…region OPEN When a region is opened, the master does not populate flushedSequenceIdByRegion until the hosting RegionServer's next heartbeat delivers a flush report. If the source RegionServer of a drain-move crashes before that heartbeat, ServerManager. getLastFlushedSequenceId returns NO_SEQNUM (-1) for the region, and WALSplitter conservatively writes already-durable edits into recovered.edits. Those orphaned edits then trigger false-positive "data loss" warnings during subsequent merge/split operations and leave regions stuck in RIT. Add ServerManager.reportRegionOpen(regionInfo, openSeqNum) and call it from AssignmentManager.regionOpenedWithoutPersistingToMeta so the watermark is established synchronously at OPEN time. putIfAbsent is used so a subsequent heartbeat with a higher completedSequenceId is never regressed by a stale open value. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Umeshkumar9414 · 2026-08-30T18:56:29Z

    regionStates.removeFromFailedOpen(regionInfo);
+    // HBASE-30335: seed the master's flushed sequence cache with openSeqNum so a subsequent
+    // WAL split (e.g. source RS crashes after drain-move) recognizes already-durable edits
+    // instead of writing orphaned recovered.edits.


It would be good to add tests.

@Umeshkumar9414 Added a unit test.

…estGetLastFlushedSequenceId New unit test TestServerManager covers reportRegionOpen behavior: - seeds flushedSequenceIdByRegion with the supplied openSeqNum; - putIfAbsent semantics prevent regressing a higher watermark that was already established (by an earlier open or a heartbeat); - NO_SEQNUM and negative openSeqNum are ignored (no-op). TestGetLastFlushedSequenceId previously asserted the pre-flush lastFlushedSequenceId was NO_SEQNUM. That assumption is invalidated by the fix (openSeqNum is now seeded synchronously on OPEN); the assertion is updated to require the watermark be present but strictly less than the memstore's earliest unflushed edit. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Apache9 · 2026-08-31T13:19:09Z

I think this is an optimization for not splitting unnecessary wal edits, but why it will lead to merge stuck? After opening a region, we should have removed all the recovered.edits files?

nirdosh0110 · 2026-08-31T13:55:32Z

I think this is an optimization for not splitting unnecessary wal edits, but why it will lead to merge stuck? After opening a region, we should have removed all the recovered.edits files?

Thanks @Apache9 for looking into it. Below is incident brief explanation. I've also raised PR8583 to reopen parent region on rollback of MERGE_TABLE_REGIONS_CHECK_CLOSED_REGIONS linked to this incident.

What Happened:

Region 112d9f08 was gracefully moved from rs-132 → rs-81 at 16:36:34 UTC.
The close on rs-132 had already flushed the region's edits, and rs-81 opened the region with openSeqNum=4997750282, establishing that the edits up to that point were durable.
~25 seconds later, rs-132 was declared dead as part of the broader graceful RS drain.
The WAL split worker (rs-34) subsequently created a recovered.edits file for this region containing edit seqId=4997750280.
This edit was already durable in the HFiles. The recovered edit file was effectively stale/orphaned, but its existence was not recognized as harmless.

Where it got stuck:

At 18:32:33 UTC, MergeTableRegionsProcedure (pid=46990253) unassigned the region successfully.
During MERGE_TABLE_REGIONS_CHECK_CLOSED_REGIONS, the procedure saw the recovered.edits file and failed the check.
The merge procedure retained the region lock and did not recover automatically, leaving the region in CLOSED/RIT for ~48m 56s.
The RIT only cleared after the HMaster failover caused the procedure to replay and fresh top-level ASSIGN procedures reopened the regions.

apurtell

Approved, with two suggestions

apurtell · 2026-08-31T16:34:57Z

+    if (openSeqNum == HConstants.NO_SEQNUM || openSeqNum < 0) {
+      return;
+    }
+    flushedSequenceIdByRegion.putIfAbsent(regionInfo.getEncodedNameAsBytes(), openSeqNum);


A max merge is more correct and equally safe, because at OPEN time a region cannot have flushed past its own openSeqNum.

public void reportRegionOpen(final RegionInfo regionInfo, final long openSeqNum) { if (openSeqNum < 0) { // NO_SEQNUM == -1 return; } flushedSequenceIdByRegion.merge(regionInfo.getEncodedNameAsBytes(), openSeqNum, Math::max); }

Addressed this comment in latest commit.

apurtell · 2026-08-31T16:38:45Z

+    // longer NO_SEQNUM before the first flush - it is the region's openSeqNum, which must
+    // still be strictly less than the memstore's earliest unflushed edit.
+    assertNotEquals(HConstants.NO_SEQNUM, ids.getLastFlushedSequenceId());
+    assertTrue(ids.getLastFlushedSequenceId() < storeSequenceId);


This assertion is fragile.
It is technically off by one, but because the region open marker consumes one seq id, the test will currently pass.

Maybe assertNotEquals(NO_SEQNUM, ...) instead?

Addressed this comment in latest commit.

@apurtell

…t assertion Per review from @apurtell: - ServerManager.reportRegionOpen: switch putIfAbsent to merge with Math::max so a stale-low prior heartbeat value is lifted to openSeqNum rather than ignored. Safe because at OPEN a region cannot have flushed past its own openSeqNum. Guard simplified to openSeqNum < 0 (NO_SEQNUM == -1, so the disjunct was redundant). - TestGetLastFlushedSequenceId: drop the strict assertTrue(lastFlushed < storeSequenceId) - it holds only because the region-open marker consumes one seqId, so the assertion is coupled to an incidental accounting detail rather than the contract being tested. The assertNotEquals(NO_SEQNUM, ...) above captures the load-bearing invariant. Reflow adjacent javadoc block for spotless.

The seed added in reportRegionOpen (openSeqNum via Math::max) exposed a long-standing test-fixture issue: AbstractTestDLS.makeWAL uses a fresh MultiVersionConcurrencyControl that stamps WAL edits starting at seqid 1, inconsistent with the seqid sequence a real WAL preserves for the region. With the seed in place, the splitter correctly filters those low-seqid edits as already-durable and testMasterStartsUpWithLogSplittingWork loses 5/1000 rows. Advance the local MVCC past the max openSeqNum of the target regions before stamping edits, so the injected WAL entries get seqids a real region would have assigned.

Apache9 · 2026-09-01T01:36:17Z

I think this is an optimization for not splitting unnecessary wal edits, but why it will lead to merge stuck? After opening a region, we should have removed all the recovered.edits files?

Thanks @Apache9 for looking into it. Below is incident brief explanation. I've also raised PR8583 to reopen parent region on rollback of MERGE_TABLE_REGIONS_CHECK_CLOSED_REGIONS linked to this incident.

What Happened:

Region 112d9f08 was gracefully moved from rs-132 → rs-81 at 16:36:34 UTC.

The close on rs-132 had already flushed the region's edits, and rs-81 opened the region with openSeqNum=4997750282, establishing that the edits up to that point were durable.

~25 seconds later, rs-132 was declared dead as part of the broader graceful RS drain.

The WAL split worker (rs-34) subsequently created a recovered.edits file for this region containing edit seqId=4997750280.

This edit was already durable in the HFiles. The recovered edit file was effectively stale/orphaned, but its existence was not recognized as harmless.

Where it got stuck:

At 18:32:33 UTC, MergeTableRegionsProcedure (pid=46990253) unassigned the region successfully. During MERGE_TABLE_REGIONS_CHECK_CLOSED_REGIONS, the procedure saw the recovered.edits file and failed the check. The merge procedure retained the region lock and did not recover automatically, leaving the region in CLOSED/RIT for ~48m 56s. The RIT only cleared after the HMaster failover caused the procedure to replay and fresh top-level ASSIGN procedures reopened the regions.

Then basically there are two problems.
On 5, we should remove the recovered.edits when opening the region, of course a failure of removing should not be considered as a critical issue.
And in MergeTableRegionsProcedure, when we have a recovered.edits file, we should check if the edits are all below the persistent seqNum, if so we are OK to remove the directory and go on.

We can do these changes in a separated issue.

Thanks.

Umeshkumar9414 · 2026-09-01T08:22:51Z

Then basically there are two problems.
On 5, we should remove the recovered.edits when opening the region, of course a failure of removing should not be considered as a critical issue.

We should check why the recovered edit files created after region open. BTW I am aware of onc case that I faced (But it looks like you faced another one) when two split worker (one zombie server and another active one) is simultaneously splitting the wal. Active one complete and we even open the region. Now zombie server can try to create one recovered-edit file from its memory.

And in MergeTableRegionsProcedure, when we have a recovered.edits file, we should check if the edits are all below the persistent seqNum, if so we are OK to remove the directory and go on.

I might not be right here but what I understand from the code is having recovered.edits files once region is open is unacceptable, and I think that was right. Even if we check that the edits are all below the persistent seqNum, we might not be sure that we didn't miss some edits. But we might not have any other way here, anyway rollback and open will do the same.

nirdosh0110 · 2026-09-01T14:58:47Z

MergeTableRegionsProcedure, when we have a recovered.edits file, we should check if the edits are all below the persistent seqNum, if so we are OK to remove the directory and go on.

@Apache9 Totally agree — we should have tolerance built into the merge and split procedures. I've filed a separate bug to track the issue pointed

…eqNum seed The test previously asserted flushedSequenceIdByRegion is byte-for-byte identical across cluster shutdown+restart. After HBASE-30335 the master seeds this map on region OPEN via merge(openSeqNum, Math::max). openSeqNum is monotonic across close/open cycles, so a region reopened after restart can carry a strictly higher value than what was persisted at shutdown, and the equality assertion no longer holds. Assert the preserved invariant instead: every region persisted at shutdown is loaded on restart (same keyset) and no watermark regresses (after[r] >= before[r]). This validates persist/load correctness without conflicting with the new seed-on-open semantic.

Copilot

🟡 Changes recommended

A concurrent heartbeat can overwrite the newly seeded watermark with an older sequence ID.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Seeds the master’s flushed-sequence cache when a region opens, preventing unnecessary WAL recovery of durable edits.

Changes:

Reports openSeqNum during region OPEN handling.
Adds unit and integration coverage.
Updates persistence and WAL-splitting tests for the new invariant.

File summaries

File	Description
`ServerManager.java`	Adds region-open sequence cache seeding.
`AssignmentManager.java`	Reports successful region opens.
`TestServerManager.java`	Tests cache seeding and monotonicity.
`TestMaster.java`	Updates restart persistence assertions.
`TestGetLastFlushedSequenceId.java`	Verifies OPEN immediately seeds the cache.
`AbstractTestDLS.java`	Aligns synthetic WAL sequence IDs with open sequence numbers.

Review details

Files reviewed: 6/6 changed files
Comments generated: 1
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Umeshkumar9414 · 2026-09-04T06:23:22Z

Can we do this while closing of the region instread of opening the region?

Looks like SCP just after the region closing is creating these recovered.edits files as RS report the flushed seq id in it heartbeats. Corrent me if I am wrong.

nirdosh0110 · 2026-09-04T13:18:05Z

Can we do this while closing of the region instread of opening the region?

Looks like SCP just after the region closing is creating these recovered.edits files as RS report the flushed seq id in it heartbeats. Corrent me if I am wrong.

CLOSE-time seeding alone doesn't cover the crash path — if the RS dies, there's no close report to seed from, so the watermark stays stale and SCP walks with the wrong fence.

Doing it at OPEN time covers both cases: a graceful close-then-reopen and a crash-then-reopen-elsewhere both go through OPEN, and openSeqNum at that point is already bumped past the close marker, so it's the strongest fence we can install.

Umeshkumar9414 · 2026-09-15T12:07:56Z

CLOSE-time seeding alone doesn't cover the crash path — if the RS dies, there's no close report to seed from, so the watermark stays stale and SCP walks with the wrong fence.

If there is no close report then master won't mvoe ahead with open so we are good in that case. We are only having a problem if edits are present after open.

nirdosh0110 · 2026-09-15T12:41:52Z

Fair — for a pure server crash we'd have stale data anyway, bounded by msginterval. RS already sends completedSequenceId for every region on every heartbeat, so master's map is continuously refreshed on the happy path, and pure-crash isn't really the differentiator between CLOSE-time and OPEN-time.

The scenario this PR is targeting is graceful move + later crash of the source RS:

t=0 — source RS cleanly closes R (flush to close-marker). Target RS opens R with openSeqNum bumped past close-marker. But master's flushedSequenceIdByRegion[R] still reflects the last heartbeat from the source RS, which was written before the close-flush landed — so it's stale.
t=25s — source RS dies. SCP walks its WAL and filters using flushedSequenceIdByRegion[R] — still the stale pre-close value. Recovered.edits are written for edits that are already durable in HFiles.
Later split/merge on R fails checkClosedRegion on those stale recovered.edits.

Both CLOSE-time and OPEN-time seeding would fix this. Reasons I kept OPEN-time:

openSeqNum ≥ close-marker, so it's an at-least-as-tight fence.
Fires on every OPEN (reassignment after SCP, master restart, RS restart), not only clean closes.
Reuses the existing reportRegionOpen payload that already carries openSeqNum — no new field on the close report.

Copilot

🔵 Needs a closer look

The change affects WAL recovery watermarks and requires human validation of concurrency and data-recovery safety.

Review details

Files reviewed: 6/6 changed files
Comments generated: 0 new
Review effort level: Balanced

apurtell · 2026-09-16T19:47:02Z

Copilot's review (2026-09-02, reiterated 2026-09-16) highlights a real race. This has to be fixed now, not deferred to another followup, otherwise what is the point. A stale in-flight heartbeat from the (soon to be dead) source RS can read null/a low value, race past the OPEN-seed, and then overwrite it with its lower value, reintroducing the bug this PR is meant to fix, just in a narrower window.

…lobber the OPEN seed updateLastFlushedSequenceIds did a non-atomic get-then-put on flushedSequenceIdByRegion. A stale in-flight heartbeat from the soon-to-be-dead source RS could read null/a low value, race past the reportRegionOpen seed (merge/Math::max), and then put its lower value on top - reintroducing the stale-fence bug this PR fixes, in a narrower window (flagged by Copilot's review). Replace both the region- and store-level updates with an atomic compute that keeps the same "never lower the watermark" rule. Every writer on the live serving path is now an atomic max-merge, so the stored value is monotonic non-decreasing and a concurrent seed can no longer be clobbered. Add TestServerManager.testConcurrentStaleHeartbeatDoesNotClobberOpenSeed, which drives the OPEN seed and a stale heartbeat concurrently over 500 rounds. It fails on the pre-fix code and passes with the fix. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

nirdosh0110 · 2026-09-16T22:13:29Z

Copilot's review (2026-09-02, reiterated 2026-09-16) highlights a real race. This has to be fixed now, not deferred to another followup, otherwise what is the point. A stale in-flight heartbeat from the (soon to be dead) source RS can read null/a low value, race past the OPEN-seed, and then overwrite it with its lower value, reintroducing the bug this PR is meant to fix, just in a narrower window.

@apurtell Got it. The race was in updateLastFlushedSequenceIds: it did a non-atomic get-then-put, so a stale in-flight heartbeat could read null/low, race past the reportRegionOpen seed, and put its lower value on top.

Both the region- and store-level updates now go through an atomic compute that keeps the existing "never lower the watermark" rule. With the merge(Math::max) seed on the OPEN side, every writer on the live serving path is now an atomic max-merge — the stored value is monotonic non-decreasing, so a concurrent seed can no longer be clobbered. Whatever the interleaving: heartbeat-before-seed → the seed lifts it to openSeqNum; heartbeat-after-seed → compute refuses to lower it.

Added TestServerManager#testConcurrentStaleHeartbeatDoesNotClobberOpenSeed to test the scenerio.

Umeshkumar9414 reviewed Aug 30, 2026

View reviewed changes

nirdosh.yadav and others added 2 commits August 31, 2026 09:38

Added unit test

cf1120c

apurtell approved these changes Aug 31, 2026

View reviewed changes

nirdosh.yadav added 3 commits September 1, 2026 06:11

HBASE-30335 spotless reflow of reportRegionOpen javadoc

095ed06

nirdosh0110 mentioned this pull request Sep 1, 2026

HBASE-30352: Tolerate stale recovered.edits below durable seqid in split/merge #8599

Open

nirdosh0110 force-pushed the HBASE-30335 branch from 1e14be3 to 7761ebf Compare September 2, 2026 07:02

mnpoonia approved these changes Sep 2, 2026

View reviewed changes

apurtell requested a balanced review from Copilot September 2, 2026 19:57

Copilot started reviewing on behalf of apurtell September 2, 2026 19:58 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java

virajjasani mentioned this pull request Sep 11, 2026

HBASE-30334: Reopen parent regions on rollback of MERGE_TABLE_REGIONS_CHECK_CLOSED_REGIONS #8583

Open

3 tasks

apurtell requested a balanced review from Copilot September 16, 2026 18:59

Copilot started reviewing on behalf of apurtell September 16, 2026 19:00 View session

Copilot AI reviewed Sep 16, 2026

View reviewed changes

Uh oh!

Conversation

nirdosh0110 commented Aug 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Test plan

Uh oh!

Umeshkumar9414 Aug 30, 2026

Choose a reason for hiding this comment

Uh oh!

nirdosh0110 Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

nirdosh0110 Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

Apache9 commented Aug 31, 2026

Uh oh!

nirdosh0110 commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apurtell left a comment

Choose a reason for hiding this comment

Uh oh!

apurtell Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

nirdosh0110 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

apurtell Aug 31, 2026

Choose a reason for hiding this comment

Uh oh!

nirdosh0110 Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

Apache9 commented Sep 1, 2026

Uh oh!

Umeshkumar9414 commented Sep 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nirdosh0110 commented Sep 1, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Umeshkumar9414 commented Sep 4, 2026

Uh oh!

nirdosh0110 commented Sep 4, 2026

Uh oh!

Umeshkumar9414 commented Sep 15, 2026

Uh oh!

nirdosh0110 commented Sep 15, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Uh oh!

apurtell commented Sep 16, 2026

Uh oh!

nirdosh0110 commented Sep 16, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nirdosh0110 commented Aug 28, 2026 •

edited

Loading

nirdosh0110 commented Aug 31, 2026 •

edited

Loading

Umeshkumar9414 commented Sep 1, 2026 •

edited

Loading