-
Notifications
You must be signed in to change notification settings - Fork 587
HDDS-3930. Fix OMKeyDeletesRequest. #1195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Thanks @bharatviswa504 for the update. LGTM, +1 from me. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bharatviswa504 for updating the patch. Looks mostly good, only 2 minor comments.
| LOG.error("Keys delete failed. Volume:{}, Bucket:{}, DeletedKey:{}, " + | ||
| "UnDeletedKeys:{}", volumeName, bucketName, keyName, | ||
| auditMap.get(DELETED_KEYS_LIST), auditMap.get(UNDELETED_KEYS_LIST), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More arguments (not counting exception) than placeholders:
| LOG.error("Keys delete failed. Volume:{}, Bucket:{}, DeletedKey:{}, " + | |
| "UnDeletedKeys:{}", volumeName, bucketName, keyName, | |
| auditMap.get(DELETED_KEYS_LIST), auditMap.get(UNDELETED_KEYS_LIST), | |
| LOG.error("Keys delete failed. Volume:{}, Bucket:{}, DeletedKeys:{}, " + | |
| "UnDeletedKeys:{}", volumeName, bucketName, | |
| auditMap.get(DELETED_KEYS_LIST), auditMap.get(UNDELETED_KEYS_LIST), |
| if (LOG.isDebugEnabled()) { | ||
| LOG.error("Keys delete failed. Volume:{}, Bucket:{}, DeletedKey:{}, " + |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: log level mismatch (isDebugEnabled vs. error).
|
Thank You @adoroszlai for the review. |
adoroszlai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @bharatviswa504 for updating the patch.
smengcl
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
d42ee30 to
7ae63c6
Compare
|
Thank You @adoroszlai @xiaoyuyao and @smengcl for the review. |
What changes were proposed in this pull request?
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-3930
Please replace this section with the link to the Apache JIRA)
How was this patch tested?
Added tests