Copilot commented on code in PR #10650:
URL: https://github.com/apache/ozone/pull/10650#discussion_r3568709103
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java:
##########
@@ -467,17 +465,37 @@ public void
addTransactions(ArrayList<DeletedBlocksTransaction> txList) throws I
incrDeletedBlocksSummary(tx);
}
}
- deletedBlockLogStateManager.addTransactionsToDB(txList, getSummary());
+ try {
+ deletedBlockLogStateManager.addTransactionsToDB(txList, getSummary());
+ } catch (IOException e) {
+ // Revert the in-memory changes if the DB update fails
+ for (DeletedBlocksTransaction tx: txList) {
+ if (tx.hasTotalBlockSize()) {
+ rollbackDeletedBlocksSummary(tx);
+ LOG.warn("{} is decreased from summary due to DB update failure",
transactionToString(tx));
+ }
+ }
+ throw e;
+ }
return;
}
deletedBlockLogStateManager.addTransactionsToDB(txList);
}
+ private void rollbackDeletedBlocksSummary(TxBlockInfo txBlockInfo) {
+ totalTxCount.addAndGet(1);
+ totalBlockCount.addAndGet(txBlockInfo.getTotalBlockCount());
+ totalBlocksSize.addAndGet(txBlockInfo.getTotalBlockSize());
+
totalReplicatedBlocksSize.addAndGet(txBlockInfo.getTotalReplicatedBlockSize());
+ LOG.debug("Increase summary for {} to {}", txBlockInfo,
summaryToString(getSummary()));
+ }
Review Comment:
LOG.debug calls summaryToString(getSummary()) directly, which eagerly builds
formatted strings even when debug logging is disabled. This adds avoidable
overhead on a hot path (transaction summary updates). Guard the debug log with
LOG.isDebugEnabled() so the String.format work only happens when needed.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java:
##########
@@ -467,17 +465,37 @@ public void
addTransactions(ArrayList<DeletedBlocksTransaction> txList) throws I
incrDeletedBlocksSummary(tx);
}
}
- deletedBlockLogStateManager.addTransactionsToDB(txList, getSummary());
+ try {
+ deletedBlockLogStateManager.addTransactionsToDB(txList, getSummary());
+ } catch (IOException e) {
+ // Revert the in-memory changes if the DB update fails
+ for (DeletedBlocksTransaction tx: txList) {
+ if (tx.hasTotalBlockSize()) {
+ rollbackDeletedBlocksSummary(tx);
+ LOG.warn("{} is decreased from summary due to DB update failure",
transactionToString(tx));
+ }
+ }
+ throw e;
+ }
return;
}
deletedBlockLogStateManager.addTransactionsToDB(txList);
}
+ private void rollbackDeletedBlocksSummary(TxBlockInfo txBlockInfo) {
+ totalTxCount.addAndGet(1);
+ totalBlockCount.addAndGet(txBlockInfo.getTotalBlockCount());
+ totalBlocksSize.addAndGet(txBlockInfo.getTotalBlockSize());
+
totalReplicatedBlocksSize.addAndGet(txBlockInfo.getTotalReplicatedBlockSize());
+ LOG.debug("Increase summary for {} to {}", txBlockInfo,
summaryToString(getSummary()));
+ }
+
private void incrDeletedBlocksSummary(DeletedBlocksTransaction tx) {
totalTxCount.addAndGet(1);
totalBlockCount.addAndGet(tx.getLocalIDCount());
totalBlocksSize.addAndGet(tx.getTotalBlockSize());
totalReplicatedBlocksSize.addAndGet(tx.getTotalBlockReplicatedSize());
+ LOG.debug("Increase summary for {} to {}", transactionToString(tx),
summaryToString(getSummary()));
}
Review Comment:
LOG.debug calls transactionToString/summaryToString directly, which eagerly
builds formatted strings even when debug logging is disabled. Guard the debug
log with LOG.isDebugEnabled() to avoid String.format overhead on frequent
summary updates.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java:
##########
@@ -590,6 +620,15 @@ private void descDeletedBlocksSummary(TxBlockInfo
txBlockInfo) {
totalBlockCount.addAndGet(-txBlockInfo.getTotalBlockCount());
totalBlocksSize.addAndGet(-txBlockInfo.getTotalBlockSize());
totalReplicatedBlocksSize.addAndGet(-txBlockInfo.getTotalReplicatedBlockSize());
+ LOG.debug("Decrease summary for {} to {}", txBlockInfo,
summaryToString(getSummary()));
Review Comment:
LOG.debug calls summaryToString(getSummary()) directly, which eagerly
formats strings even when debug logging is disabled. Guard with
LOG.isDebugEnabled() to avoid unnecessary String.format overhead during
transaction removals.
##########
hadoop-hdds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/block/SCMDeletedBlockTransactionStatusManager.java:
##########
@@ -590,6 +620,15 @@ private void descDeletedBlocksSummary(TxBlockInfo
txBlockInfo) {
totalBlockCount.addAndGet(-txBlockInfo.getTotalBlockCount());
totalBlocksSize.addAndGet(-txBlockInfo.getTotalBlockSize());
totalReplicatedBlocksSize.addAndGet(-txBlockInfo.getTotalReplicatedBlockSize());
+ LOG.debug("Decrease summary for {} to {}", txBlockInfo,
summaryToString(getSummary()));
+ }
+
+ private void rollbackDeletedBlocksSummary(DeletedBlocksTransaction tx) {
+ totalTxCount.addAndGet(-1);
+ totalBlockCount.addAndGet(-tx.getLocalIDCount());
+ totalBlocksSize.addAndGet(-tx.getTotalBlockSize());
+ totalReplicatedBlocksSize.addAndGet(-tx.getTotalBlockReplicatedSize());
+ LOG.debug("Decrease summary for {} to {}", transactionToString(tx),
summaryToString(getSummary()));
}
Review Comment:
LOG.debug calls transactionToString/summaryToString directly, which eagerly
formats strings even when debug logging is disabled. Guard with
LOG.isDebugEnabled() so the String.format work only happens when debug is
enabled.
--
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]
