apurtell commented on code in PR #8584:
URL: https://github.com/apache/hbase/pull/8584#discussion_r3896401536
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/ServerManager.java:
##########
@@ -1092,6 +1092,22 @@ 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 putIfAbsent} so a heartbeat-supplied value (which may reflect
flushes after open) is
+ * never regressed. See HBASE-30335.
+ */
+ public void reportRegionOpen(final RegionInfo regionInfo, final long
openSeqNum) {
+ if (openSeqNum == HConstants.NO_SEQNUM || openSeqNum < 0) {
+ return;
+ }
+ flushedSequenceIdByRegion.putIfAbsent(regionInfo.getEncodedNameAsBytes(),
openSeqNum);
Review Comment:
A max merge is more correct and equally safe, because at OPEN time a region
cannot have flushed past its own `openSeqNum`.
```java
public void reportRegionOpen(final RegionInfo regionInfo, final long
openSeqNum) {
if (openSeqNum < 0) { // NO_SEQNUM == -1
return;
}
flushedSequenceIdByRegion.merge(regionInfo.getEncodedNameAsBytes(),
openSeqNum, Math::max);
}
```
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestGetLastFlushedSequenceId.java:
##########
@@ -89,10 +91,14 @@ public void test() throws IOException, InterruptedException
{
Thread.sleep(2000);
RegionStoreSequenceIds ids =
testUtil.getHBaseCluster().getMaster().getServerManager()
.getLastFlushedSequenceId(region.getRegionInfo().getEncodedNameAsBytes());
- assertEquals(HConstants.NO_SEQNUM, ids.getLastFlushedSequenceId());
// This will be the sequenceid just before that of the earliest edit in
memstore.
long storeSequenceId = ids.getStoreSequenceId(0).getSequenceId();
assertTrue(storeSequenceId > 0);
+ // HBASE-30335: openSeqNum is now seeded on region OPEN, so
lastFlushedSequenceId is no
+ // 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);
Review Comment:
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?
--
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]
