HDDS-10029. Improved logs for SCMDeletedBlockTransactionStatusManager #5888
Conversation
|
@swamirishi can you please take a look? |
| break; | ||
| default: | ||
| LOG.error("Can not update to Unknown new Status: {}", newStatus); | ||
| LOG.error("Unable to update from unknown DN report status: {}", newStatus); |
There was a problem hiding this comment.
Thanks for the patch @xichen01. Shouldn't the log here say Unable to update to <status> instead of Unable to update from <status>?
Even in the above log changes for updateStatus() it's not really clear if DN report status is the new status that the SCM command should be updated to. May I ask what was unclear in the old log messages?
There was a problem hiding this comment.
This has been updated.
| if (updateTime != null && | ||
| Duration.between(updateTime, now).toMillis() > timeoutMs) { | ||
| CmdStatusData state = removeScmCommand(dnId, scmCmdId); | ||
| LOG.warn("Remove Timeout SCM BlockDeletionCommand {} for DN {} " + |
There was a problem hiding this comment.
I think the log message here could be updated to LOG.warn("SCM BlockDeletionCommand {} for DN {} was removed after {}ms without update", state, dnId, timeoutMs). Let me know what you think?
There was a problem hiding this comment.
Thanks @xichen01 for working on this.
These messages include the same information with different prefix label. E.g. SCM command ID appears as:
scmCmdId {}SCM Command: {}SCM Command ID: {}ScmCommandId {}
which makes it difficult to filter the log for this information.
Similar problem with datanode ID.
I suggest standardizing it.
Standardized LOG format to |
|
@SaketaChalamchala would you like to take another look? |
|
Thanks @xichen01 for the patch, @SaketaChalamchala, @sumitagrawl for the review. |
…apache#5888) (cherry picked from commit 41ea9fb)
…apache#5888) (cherry picked from commit 41ea9fb)
…apache#5888) (cherry picked from commit 41ea9fb)
4 participants
What changes were proposed in this pull request?
Improved logs for
SCMDeletedBlockTransactionStatusManager.SCMDeletedBlockTransactionStatusManagersometimes outputs some redundant WARN logs or unclear logs.What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10029
How was this patch tested?
Existing Test