HDDS-15726. Improve SCM block deletion summary error handling#10650
Conversation
priyeshkaratha
left a comment
priyeshkaratha
left a comment
There was a problem hiding this comment.
Thank you @ChenSammi for working on this. Changes LGTM. Just have minor suggestion on newly added method name.
| deletedBlockLogStateManager.addTransactionsToDB(txList); | ||
| } | ||
|
|
||
| private void incrDeletedBlocksSummary(TxBlockInfo txBlockInfo) { |
There was a problem hiding this comment.
since this method is used to do rollback, can we rename to something like restoreDeletedBlocksSummary
aryangupta1998
left a comment
aryangupta1998
left a comment
There was a problem hiding this comment.
Thanks for the patch, @ChenSammi, some minor comments inline!
There was a problem hiding this comment.
Pull request overview
This PR improves SCM deleted-block transaction summary handling by ensuring commit processing only happens when the SCM leader is ready, and by keeping the in-memory deletion summary consistent when DB persistence fails.
Changes:
- Gate
DeletedBlockLogImpl#onMessageprocessing onscmContext.isLeaderReady(). - Add rollback logic in
SCMDeletedBlockTransactionStatusManagerto revert summary / txSizeMap updates when DB operations fail. - Add additional logging and enrich
TxBlockInfowithtxIdandcontainerIdfor better diagnostics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| hadoop-hdds/server-scm/src/test/java/org/apache/hadoop/hdds/scm/block/TestDeletedBlockLog.java | Updates test construction of TxBlockInfo for the new constructor signature. |
| hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java | Adds rollback-on-DB-failure behavior, improves summary initialization logging, and expands TxBlockInfo to carry IDs. |
| hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/DeletedBlockLogImpl.java | Adds leader-ready gating and updates tx-size map population to use the new TxBlockInfo fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
priyeshkaratha
left a comment
priyeshkaratha
left a comment
There was a problem hiding this comment.
Thanks @ChenSammi for updating the patch. Changes LGTM +1
|
@aryangupta1998 , could you take another look too? |
aryangupta1998
left a comment
aryangupta1998
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments @ChenSammi, LGTM!
|
Thanks for the patch, @ChenSammi and @priyeshkaratha, for the review! |
What changes were proposed in this pull request?
Improve the block deletion summary error handing
What is the link to the Apache JIRA
HDDS-15726
How was this patch tested?
existing unit tests.