smengcl commented on code in PR #11187:
URL: https://github.com/apache/ozone/pull/11187#discussion_r4311410042
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java:
##########
@@ -486,6 +516,13 @@ public void
addTransactions(ArrayList<DeletedBlocksTransaction> txList) throws I
deletedBlockLogStateManager.addTransactionsToDB(txList);
}
+ private void checkSummaryReloaded() throws SCMException {
+ if (summaryReloadPending) {
+ throw new SCMException("Deleted blocks transaction summary is not
reloaded yet after becoming leader",
+ SCMException.ResultCodes.SCM_NOT_LEADER);
+ }
+ }
Review Comment:
After a failover, `deleteKeyBlocks` can fail for a short time because of
this check.
Ratis reports the leader as ready (`checkLeader()` is true) before it calls
`notifyLeaderReady` (`LeaderStateImpl#checkReady`). Until `notifyLeaderReady`
finishes the flush and reload, `summaryReloadPending` is still true. In that
window SCM accepts `deleteKeyBlocks` and then rejects it here. OM gets
`unknownFailure` for the whole batch, and SCM logs a WARN with a stack trace.
On master the same request waits in Ratis and succeeds.
OM retries the batch later, so no data is lost. Pls either:
1. keep the rejection and add a comment saying this window is expected, or
2. wait here for the reload, with a timeout, instead of failing right away.
For option 2, `onLeaderReady` takes only `summaryLock`, and `wait()`
releases it, so this does not deadlock with `DeletedBlockLogImpl.lock`:
```suggestion
private void checkSummaryReloaded() throws SCMException {
long deadline = Time.monotonicNow() + SUMMARY_RELOAD_WAIT_MS;
while (summaryReloadPending) {
long remaining = deadline - Time.monotonicNow();
if (remaining <= 0) {
throw new SCMException("Deleted blocks transaction summary is not
reloaded yet after becoming leader",
SCMException.ResultCodes.SCM_NOT_LEADER);
}
try {
summaryLock.wait(remaining);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new SCMException("Interrupted while waiting for summary
reload", e,
SCMException.ResultCodes.INTERNAL_ERROR);
}
}
}
```
Option 2 also needs:
- `summaryLock.notifyAll()` wherever `summaryReloadPending` is cleared
(`initDataDistributionData` and the `onLeaderReady` catch block)
- a `SUMMARY_RELOAD_WAIT_MS` constant
- a way for tests to shorten the wait, since
`testSummaryUpdateRejectedUntilLeaderReady` would otherwise wait the full
timeout on each call
- a test that an add waiting on the reload goes through after
`onLeaderReady()`
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManagerImpl.java:
##########
@@ -146,11 +146,18 @@ public void removeFromDB() {
@Override
public void addTransactionsToDB(ArrayList<DeletedBlocksTransaction> txs,
DeletedBlocksTransactionSummary summary) throws IOException {
- for (DeletedBlocksTransaction tx : txs) {
- transactionBuffer.addToBuffer(deletedTable, tx.getTxID(), tx);
- }
- if (summary != null) {
- transactionBuffer.addToBuffer(statefulConfigTable, SERVICE_NAME,
summary.toByteString());
+ // Hold the buffer lock across the row-plus-summary write so that a
concurrent flush() (e.g. from takeSnapshot)
+ // cannot land between the two, which would commit the transaction rows to
RocksDB without the matching summary.
Review Comment:
nit
```suggestion
// Hold the buffer lock across the row-plus-summary write so that a
concurrent flush() (e.g. from a checkpoint
// download) cannot land between the two, which would commit the
transaction rows to RocksDB without the summary.
```
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogStateManagerImpl.java:
##########
@@ -146,11 +146,18 @@ public void removeFromDB() {
@Override
public void addTransactionsToDB(ArrayList<DeletedBlocksTransaction> txs,
DeletedBlocksTransactionSummary summary) throws IOException {
- for (DeletedBlocksTransaction tx : txs) {
- transactionBuffer.addToBuffer(deletedTable, tx.getTxID(), tx);
- }
- if (summary != null) {
- transactionBuffer.addToBuffer(statefulConfigTable, SERVICE_NAME,
summary.toByteString());
+ // Hold the buffer lock across the row-plus-summary write so that a
concurrent flush() (e.g. from takeSnapshot)
+ // cannot land between the two, which would commit the transaction rows to
RocksDB without the matching summary.
+ transactionBuffer.lock();
Review Comment:
No test covers this lock. Add one next to
`TestSCMHATransactionBufferMonitorTask#testRemoveTransactionsFromDBBlocksConcurrentFlushUntilDurable`:
block the writer after the row `addToBuffer`, start `flush()` on another
thread, and assert that it is `WAITING` and that neither the row nor the
summary is durable. Then release the writer and assert that both are durable.
--
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]
