rich7420 commented on code in PR #11188:
URL: https://github.com/apache/ozone/pull/11188#discussion_r3932278937
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManagerImpl.java:
##########
@@ -157,14 +157,23 @@ public void
addTransactionsToDB(ArrayList<DeletedBlocksTransaction> txs,
@Override
public void removeTransactionsFromDB(ArrayList<Long> txIDs,
DeletedBlocksTransactionSummary summary)
throws IOException {
- if (deletingTxIDs != null) {
- deletingTxIDs.addAll(txIDs);
- }
- for (Long txID : txIDs) {
- transactionBuffer.removeFromBuffer(deletedTable, txID);
- }
- if (summary != null) {
- transactionBuffer.addToBuffer(statefulConfigTable, SERVICE_NAME,
summary.toByteString());
+ // Hold the buffer lock across the whole mark-remove-summary sequence so
that a concurrent flush()
+ // (checkpoint download or leader transfer) cannot land between marking
these txIDs as hidden and their
+ // removal being durably flushed. Otherwise onFlush() would reset
deletingTxIDs while the row is still
+ // present, re-exposing it to the deletion scanner and causing the summary
to be double-decremented.
+ transactionBuffer.lock();
Review Comment:
This can still combine two different flush epochs. `deletedTable.iterator()`
is initialized before `snapshotDeletingTxIDs`;
[`RDBTable.iterator()`](https://github.com/apache/ozone/blob/3d65ad89310621211e727d21ca6bc0b82c506c0a/hadoop-hdds/framework/src/main/java/org/apache/hadoop/hdds/utils/db/RDBTable.java#L221-L225)
creates the underlying RocksDB iterator at that point. If `flush()` runs
between those two initializers, the iterator retains the deleted row while
`onFlush()` replaces `deletingTxIDs` with a new empty set, so the row is
returned again.
I reproduced this by pausing after iterator creation, flushing the buffered
delete, then resuming:
```
[ERROR] A durably deleted transaction was re-exposed
expected: <false> but was: <true>
```
Reversing the two initializers made the same test pass. Please capture
`deletingTxIDs` before creating the iterator, or hold the buffer read lock
while capturing both, and cover this interleaving in the regression test.
##########
hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/ha/TestSCMHATransactionBufferMonitorTask.java:
##########
@@ -272,4 +288,122 @@ public void
testMonitorTaskDoesNotPartialFlushDuringTransactionApply()
assertEquals(ByteString.copyFromUtf8("value"),
statefulServiceConfigTable.get("key"));
}
+
+ /**
+ * Verifies that {@link SCMHADBTransactionBuffer#lock()} establishes mutual
exclusion with
+ * {@link SCMHADBTransactionBufferImpl#flush()}'s write lock (HDDS-16145).
+ */
+ @Test
+ public void testLockBlocksConcurrentFlush() throws Exception {
+ transactionBuffer.updateLatestTrxInfo(TRX_INFO_T4);
+ transactionBuffer.flush();
+
+ transactionBuffer.lock();
Review Comment:
nit: please release this lock in a `finally` block. If `waitFor` or the
assertion fails before `unlock()`, the non-daemon flusher remains parked on the
write lock until the test fork terminates.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
