virajjasani commented on code in PR #8707:
URL: https://github.com/apache/hbase/pull/8707#discussion_r4254817461
##########
hbase-server/src/main/java/org/apache/hadoop/hbase/master/assignment/AssignmentManager.java:
##########
@@ -1230,8 +1230,17 @@ private void
reportRegionStateTransition(ReportRegionStateTransitionResponse.Bui
final RegionInfo hri =
ProtobufUtil.toRegionInfo(transition.getRegionInfo(0));
long procId =
transition.getProcIdCount() > 0 ? transition.getProcId(0) :
Procedure.NO_PROC_ID;
- updateRegionTransition(serverNode, transition.getTransitionCode(),
hri,
- transition.hasOpenSeqNum() ? transition.getOpenSeqNum() :
HConstants.NO_SEQNUM, procId);
+ long seqId =
+ transition.hasOpenSeqNum() ? transition.getOpenSeqNum() :
HConstants.NO_SEQNUM;
+ // On CLOSE, seed the master's flushed-seqid watermark with the
region's final flushed
+ // seqid (reported by the RS). This complements the OPEN-time seed
(HBASE-30335) and
+ // narrows the graceful-close window where a WAL split of a crashed
source RS could
+ // write orphaned recovered.edits for already-durable edits. Uses
the same monotonic
+ // merge(Math::max) seed, so it never regresses a higher value.
+ if (transition.getTransitionCode() == TransitionCode.CLOSED && seqId
>= 0) {
+ master.getServerManager().reportRegionOpen(hri, seqId);
Review Comment:
Since now region OPEN, FAILED_OPEN and CLOSED cases use the same
ServerManager API, instead of calling it `reportRegionOpen()`, we can rename it
to something generic like `advanceRegionFlushedSeq()`.
##########
hbase-server/src/test/java/org/apache/hadoop/hbase/master/TestGetLastFlushedSequenceId.java:
##########
@@ -143,4 +143,60 @@ public void testFlushedSequenceIdSeededOnRegionOpen()
throws IOException, Interr
table.close();
}
}
+
+ /**
+ * HBASE-30335 follow-up: on graceful region CLOSE the RS reports the
region's final durable
+ * flushed seqid, and the master must lift its flushedSequenceIdByRegion
watermark to that value.
+ * This complements the OPEN-time seed and covers the drain-move / disable
window: if the source
+ * RS crashes after the close, a subsequent WAL split must see the
already-durable edits as
+ * flushed and not resurrect them as orphaned recovered.edits. Writes and
flushes so the region's
+ * maxFlushedSeqId advances well past its openSeqNum, then disables the
table (a graceful close
+ * that keeps the region - unlike delete/split/merge, disable does not call
+ * ServerManager#removeRegion) and asserts the watermark reflects the
post-close flushed seqid.
+ */
+ @Test
+ public void testFlushedSequenceIdSeededOnRegionClose() throws IOException,
InterruptedException {
+ TableName freshTable = TableName.valueOf(getClass().getSimpleName(),
"closeseed");
+ testUtil.getAdmin()
+
.createNamespace(NamespaceDescriptor.create(freshTable.getNamespaceAsString()).build());
+ Table table = testUtil.createTable(freshTable, families);
+ try {
+ SingleProcessHBaseCluster cluster = testUtil.getMiniHBaseCluster();
+ HRegion region = null;
+ for (JVMClusterUtil.RegionServerThread rst :
cluster.getRegionServerThreads()) {
+ for (HRegion r : rst.getRegionServer().getRegions(freshTable)) {
+ region = r;
+ break;
+ }
+ if (region != null) {
+ break;
+ }
+ }
+ assertNotNull(region);
+ long openSeqNum = region.getOpenSeqNum();
+ // Write and flush a few times so the region's durable flushed seqid
advances past openSeqNum;
+ // this makes the CLOSE-time seed distinguishable from the OPEN-time
seed.
+ for (int i = 0; i < 3; i++) {
+ table.put(new Put(Bytes.toBytes("k" + i)).addColumn(family,
Bytes.toBytes("q"),
+ Bytes.toBytes("v" + i)));
+ testUtil.getAdmin().flush(freshTable);
+ }
+ long flushedSeqId = region.getMaxFlushedSeqId();
+ assertTrue(flushedSeqId > openSeqNum,
+ "test setup: flushed seqid " + flushedSeqId + " must exceed openSeqNum
" + openSeqNum);
+ byte[] encodedName = region.getRegionInfo().getEncodedNameAsBytes();
+ // Gracefully close the region (disable keeps the region entry - it is
not removed like a
+ // delete/split/merge would), driving a CLOSED transition that carries
the flushed seqid.
+ testUtil.getAdmin().disableTable(freshTable);
+ RegionStoreSequenceIds ids =
testUtil.getHBaseCluster().getMaster().getServerManager()
+ .getLastFlushedSequenceId(encodedName);
+ assertNotEquals(HConstants.NO_SEQNUM, ids.getLastFlushedSequenceId(),
+ "flushedSequenceIdByRegion should be seeded on region CLOSE");
+ assertTrue(ids.getLastFlushedSequenceId() >= flushedSeqId,
+ "CLOSE seed " + ids.getLastFlushedSequenceId() + " must be >= the
region's flushed seqid "
+ + flushedSeqId);
Review Comment:
After retrieving `flushedSeqId`, since we are only disabling the table, is
it possible for `ids.getLastFlushedSequenceId()` to be greater than
`flushedSeqId`? Or they both should be same?
--
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]
