errose28 commented on code in PR #6547:
URL: https://github.com/apache/ozone/pull/6547#discussion_r1569233154
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -626,20 +630,27 @@ private void updateMetaData(KeyValueContainerData
containerData,
}
}
- private void logDeleteTransaction(long containerId,
- KeyValueContainerData containerData, DeletedBlocksTransaction delTX) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Processing Container : {}, DB path : {}, transaction {}",
- containerId, containerData.getMetadataPath(), delTX.getTxID());
- }
+ public static boolean isDuplicateTransaction(long containerId,
KeyValueContainerData containerData,
+ DeletedBlocksTransaction delTX, BlockDeletingServiceMetrics metrics) {
+ boolean duplicate = false;
- if (delTX.getTxID() <= containerData.getDeleteTransactionId()) {
- blockDeleteMetrics.incOutOfOrderDeleteBlockTransactionCount();
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ metrics.incOutOfOrderDeleteBlockTransactionCount();
LOG.info(String.format("Delete blocks for containerId: %d"
- + " is either received out of order or retried,"
- + " %d <= %d", containerId, delTX.getTxID(),
+ + " is received out of order, %d < %d", containerId,
delTX.getTxID(),
containerData.getDeleteTransactionId()));
+ } else if (delTX.getTxID() == containerData.getDeleteTransactionId()) {
+ duplicate = true;
+ LOG.info(String.format("Delete blocks with txID {} for containerId: %d"
+ + " is retried.", delTX.getTxID(), containerId,
+ containerData.getDeleteTransactionId()));
Review Comment:
Should we make this retry case debug instead? It looks kind of verbose in
the Jira shared and I don't think the information is very useful.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/DeleteBlocksCommandHandler.java:
##########
@@ -626,20 +630,27 @@ private void updateMetaData(KeyValueContainerData
containerData,
}
}
- private void logDeleteTransaction(long containerId,
- KeyValueContainerData containerData, DeletedBlocksTransaction delTX) {
- if (LOG.isDebugEnabled()) {
- LOG.debug("Processing Container : {}, DB path : {}, transaction {}",
- containerId, containerData.getMetadataPath(), delTX.getTxID());
- }
+ public static boolean isDuplicateTransaction(long containerId,
KeyValueContainerData containerData,
+ DeletedBlocksTransaction delTX, BlockDeletingServiceMetrics metrics) {
+ boolean duplicate = false;
- if (delTX.getTxID() <= containerData.getDeleteTransactionId()) {
- blockDeleteMetrics.incOutOfOrderDeleteBlockTransactionCount();
+ if (delTX.getTxID() < containerData.getDeleteTransactionId()) {
+ metrics.incOutOfOrderDeleteBlockTransactionCount();
LOG.info(String.format("Delete blocks for containerId: %d"
- + " is either received out of order or retried,"
- + " %d <= %d", containerId, delTX.getTxID(),
+ + " is received out of order, %d < %d", containerId,
delTX.getTxID(),
containerData.getDeleteTransactionId()));
+ } else if (delTX.getTxID() == containerData.getDeleteTransactionId()) {
+ duplicate = true;
+ LOG.info(String.format("Delete blocks with txID {} for containerId: %d"
+ + " is retried.", delTX.getTxID(), containerId,
+ containerData.getDeleteTransactionId()));
Review Comment:
I guess one case for making this info is that we can see if block delete is
running slow and SCM is frequently retrying. Although if we suspect this is
continuing for a while we can enable debug logging for this class as needed.
##########
hadoop-hdds/container-service/src/test/java/org/apache/hadoop/ozone/container/common/statemachine/commandhandler/TestDeleteBlocksCommandHandler.java:
##########
@@ -347,7 +379,11 @@ private static class TestSchemaHandler implements
SchemaHandler {
@Override
public void handle(KeyValueContainerData containerData,
DeletedBlocksTransaction tx) throws IOException {
- // doNoting just for Test
+ if
(DeleteBlocksCommandHandler.isDuplicateTransaction(containerData.getContainerID(),
containerData, tx, null)) {
+ return;
+ }
Review Comment:
This doesn't quite test the whole scope of the change, but I think for this
simple change it is ok given the existing structure of the test. It would be
good to have this test run with a configuration that creates the containers in
a tmp directory and mocked `OzoneContainer` object so we can test the whole
`DeleteBlocksCommandHandler` object, but that might be out of scope for this
task.
--
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]
