smengcl commented on code in PR #11187:
URL: https://github.com/apache/ozone/pull/11187#discussion_r4099828347
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java:
##########
@@ -461,13 +465,18 @@ public void
addTransactions(ArrayList<DeletedBlocksTransaction> txList) throws I
}
if
(VersionedDatanodeFeatures.isFinalized(HDDSLayoutFeature.STORAGE_SPACE_DISTRIBUTION)
&&
!disableDataDistributionForTest) {
- for (DeletedBlocksTransaction tx: txList) {
- if (tx.hasTotalBlockSize()) {
- incrDeletedBlocksSummary(tx);
+ DeletedBlocksTransactionSummary summary;
+ synchronized (summaryLock) {
+ for (DeletedBlocksTransaction tx: txList) {
+ if (tx.hasTotalBlockSize()) {
+ incrDeletedBlocksSummary(tx);
+ }
}
+ onSummaryUpdatedForTest();
+ summary = getSummary();
}
Review Comment:
`summaryLock` is released before the updated summary is buffered. A leader
change can reload the previous durable summary in that gap and overwrite the
new counters. The remove path has the same issue.
Pls coordinate the reload with the full update and buffer operation, and
test the final in-memory counters.
Pls guard the complete update and buffer operation, and test the final
in-memory summary.
```diff
diff --git
a/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockSummaryFlushRace.java
b/hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockSummaryFlushRace.java
@@ -338,6 +338,8 @@
"The summary handed to addTransactionsToDB (what gets durably
persisted) must reflect "
+ "Tx2's increment. BUG: a concurrent onBecomeLeader reset
landed between the "
+ "counter update and getSummary(), so a stale summary would
have been persisted.");
+ assertEquals(2, statusManager.getSummary().getTotalTransactionCount(),
+ "Leader reload must not discard Tx2's in-memory increment");
}
@@ -406,6 +408,8 @@
"The summary handed to removeTransactionsFromDB (what gets durably
persisted) must "
+ "reflect Tx2's removal. BUG: a concurrent onBecomeLeader
reset landed between the "
+ "counter update and getSummary(), so a stale summary would
have been persisted.");
+ assertEquals(1, statusManager.getSummary().getTotalTransactionCount(),
+ "Leader reload must not discard Tx2's in-memory decrement");
}
```
--
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]
