HDDS-12175. Audit logs in SCM shouldn't print delete txns #7805
Conversation
There was a problem hiding this comment.
Thanks @Tejaskriya for the patch.
Please provide examples of audit log before/after the change, can take it from acceptance test logs (before, after).
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java
Outdated
Show resolved
Hide resolved
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java
Outdated
Show resolved
Hide resolved
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java
Outdated
Show resolved
Hide resolved
|
@adoroszlai Thanks for the review! I have made the changes and added the old and new audit log in the PR description. |
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java
Outdated
Show resolved
Hide resolved
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java
Outdated
Show resolved
Hide resolved
...ds/server-scm/src/main/java/org/apache/hadoop/hdds/scm/server/SCMDatanodeProtocolServer.java
Outdated
Show resolved
Hide resolved
|
@adoroszlai I have addressed all the comments, Thanks for suggesting these improvements! |
There was a problem hiding this comment.
Thanks @Tejaskriya for working on this.
Overall the change looks good.
Let's retain cmdId, encodedToken and deadlineMsSinceEpoch in the audit log for deleteBlocksCommand as well.
Also, please add a debug level log which prints the transactions. We don't have to print the block IDs (Local IDs), instead we can print the total number of blocks in a container.
|
You can add the debug log in |
|
Thanks for the review @nandakumar131 , I have added the extra details to the audit log and also added a debug log in |
| allCommands.append("cmdID: ").append(cmd.getId()); | ||
| allCommands.append(" encodedToken: \"").append(cmd.getEncodedToken()).append("\""); | ||
| allCommands.append(" term: ").append(cmd.getTerm()); | ||
| allCommands.append(" deadlineMsSinceEpoch: ").append(cmd.getDeadline()); | ||
| if (cmd.getType().equals(deleteBlocksCommand)) { | ||
| DeleteBlocksCommand delCmd = (DeleteBlocksCommand) cmd; | ||
| allCommands.append(" deleteBlocksTransactions: {"); | ||
| for (DeletedBlocksTransaction txn : delCmd.blocksTobeDeleted()) { | ||
| allCommands.append(" txnId:").append(txn.getTxID()); | ||
| allCommands.append(" containerID:").append(txn.getContainerID()); | ||
| allCommands.append(" deleteBlockCount:").append(txn.getLocalIDCount()); | ||
| allCommands.append(" count:").append(txn.getCount()); | ||
| allCommands.append(","); | ||
| } | ||
| allCommands.deleteCharAt(allCommands.length() - 1); | ||
| allCommands.append(" }"); | ||
| } else { | ||
| allCommands.append(" ").append(cmd.getProto()); | ||
| } | ||
| allCommands.append(", "); | ||
| } | ||
| int len = allCommands.length(); | ||
| allCommands.delete(len - 2, len); |
There was a problem hiding this comment.
SCMCommand is not a protobuf object, you can override toString method in all the SCMCommand implementation and just logging the SCMCommand#toString here should be enough.
What changes were proposed in this pull request?
As a part of SCM audits for datanode's heartbeats, the command.toString for the commands being sent to the datanodes are logged. For most commands this results in a short string, but for deleteBlocks command, all the delete transactions are also added. This leads to a very verbose audit log.
SCM should add only the metadata response to the logs appropriately for each of the commands.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-12175
How was this patch tested?
Tested manually.
Before the change:
After the change:
The debug log added: