HDDS-9462. DeleteBlocksCommandHandler should not print TXID at info level

[shivaom02]

What changes were proposed in this pull request?

DeleteBlocksCommandHandler should not print TXID at info level.

Basically, the DeleteBlocksCommandHandler was logging all the delete block transaction IDs at info level, and thus flooding the datanode log.
Now, the transaction IDs are logged only if the Debug node is enabled, otherwise only the number of transactions are logged at info level.
This is done so that user is not overwhelmed with unimportant information, rather have cleaner and more relevant information.

What is the link to the Apache JIRA

HDDS-9462

How was this patch tested?

As it is only a logging change, no new test cases were added. But the existing tests for this module were run successfully.

[adoroszlai]

Thanks @shivaom02 for the patch. Please enable the build-branch workflow in your fork.

[shivaom02]

Thanks @adoroszlai , I've enabled all workflows from my fork.

[sodonnel]

Can we remove the if (LOG.isDebugEnabled()) and just change the log message to debug level, ie:

LOG.debug("Start to delete container blocks, TXIDs={}",
summary.getTxIDSummary());

Inside the logger, it applies the if (LOG.isDebugEnabled()) so if we just use a debug level log, then we can remove the extra if statement.

[shivaom02]

Sure, thank you.

[adoroszlai]

Unfortunately we're better off using if (LOG.isDebugEnabled()) in this case. getTxIDSummary() performs quite a bit of stream/string processing, which we would like to avoid if the log is going to be discarded.

ozone/hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/helpers/DeletedContainerBlocksSummary.java

Lines 93 to 98 in 47f472f

	publicStringgetTxIDSummary() {
	List<String> txSummaryEntry = txSummary.entrySet().stream()
	.map(entry -> entry.getKey() + "(" + entry.getValue() + ")")
	.collect(Collectors.toList());
	return"[" + String.join(",", txSummaryEntry) + "]";
	}

[adoroszlai]

enabled all workflows from my fork

Great. Note: if you push multiple commits for the same branch in your fork, you might want to cancel runs for obsolete commits you no longer care about.

[sodonnel]

LGTM pending green CI.

[shivaom02]

enabled all workflows from my fork

Great. Note: if you push multiple commits for the same branch in your fork, you might want to cancel runs for obsolete commits you no longer care about.

Okay, Thanks @adoroszlai

[shivaom02]

LGTM pending green CI.

Thanks for the review @sodonnel. The CI checks have passed.

[adoroszlai]

Thanks @shivaom02 for updating the patch.

In addition to restoring isDebugEnabled(), I would also consider moving the detailed debug message after the info-level summary one.

[shivaom02]

I've made the changes, please approve the workflows @adoroszlai

[shivaom02]

@adoroszlai, I had an indentation error, I've fixed it. Please approve the workflow.

[sodonnel]

Ah of course. I missed the expense of creating the passed parameter to the debug message.

[adoroszlai]

Thanks @shivaom02 for the patch, @sodonnel for the review.

[shivaom02]

Thank you so much @adoroszlai .