nirdosh0110 commented on code in PR #8584:
URL: https://github.com/apache/hbase/pull/8584#discussion_r3934210025
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java:
##########
@@ -1092,6 +1092,24 @@ public void removeRegion(final RegionInfo regionInfo) {
flushedSequenceIdByRegion.remove(encodedName);
}
+ /**
+ * Called on region OPEN to seed {@link #flushedSequenceIdByRegion} with the
region's
+ * {@code openSeqNum}. Without this, the entry stays absent until the
hosting server's next
+ * heartbeat, so {@link #getLastFlushedSequenceId} returns {@link
HConstants#NO_SEQNUM} and
+ * WALSplitter conservatively treats already-durable edits as unflushed -
producing orphaned
+ * recovered.edits when the source server crashes soon after a drain-move.
Uses {@code merge} with
+ * {@link Math#max} so a heartbeat-supplied value (which may reflect flushes
after open) is never
+ * regressed - and, unlike {@code putIfAbsent}, a stale-low prior value is
lifted to
+ * {@code openSeqNum}. Safe because at OPEN a region cannot have flushed
past its own
+ * {@code openSeqNum}. See HBASE-30335.
+ */
+ public void reportRegionOpen(final RegionInfo regionInfo, final long
openSeqNum) {
+ if (openSeqNum < 0) { // NO_SEQNUM == -1
+ return;
+ }
+ flushedSequenceIdByRegion.merge(regionInfo.getEncodedNameAsBytes(),
openSeqNum, Math::max);
Review Comment:
Good catch — the race is real. `updateLastFlushedSequenceIds` reads
`flushedSequenceIdByRegion` at line 292 and does a conditional `put` at line
300 without holding any lock, so the following interleaving is possible:
1. Stale heartbeat thread reads `existingValue = null`.
2. `reportRegionOpen(regionInfo, openSeqNum=10)` executes its atomic
`merge(Math::max)` → map now has `10`.
3. Stale heartbeat thread resumes and writes `completedSeqId = 3` → map
regresses to `3`.
The read-then-put pattern predates this PR, but HBASE-30335 sharpens the
exposure. Before this change every writer to the map was the same heartbeat
path carrying monotonically-nondecreasing values from a single RS — a lost
update was self-healing on the next heartbeat. With the new OPEN-time seed, the
writer values are heterogeneous (a fresh `openSeqNum` after reopen can be
strictly greater than any stale RS's `completedSequenceId` observed during
graceful drain / failover), and a regression on this path is not self-healing
because the OPEN seed happens once per region open, not periodically.
Filing this as a follow-up JIRA to keep this PR's diff focused on the seed
installation and to make bisection precise if either change regresses. The
follow-up will convert both the region-level and per-store read-then-put pairs
to atomic `compute(...)` and add a concurrency test that races
`reportRegionOpen(high)` against a heartbeat-shaped `regionServerReport(low)`
and asserts the final map holds `high`. Will link the follow-up PR here once
it's up.
--
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]
