dineshchitlangia commented on a change in pull request #1734:
URL: https://github.com/apache/ozone/pull/1734#discussion_r555127920
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAddAclRequest.java
##########
@@ -116,7 +126,11 @@ OMClientResponse onFailure(OMResponse.Builder omResponse,
@Override
void onComplete(boolean operationResult, IOException exception,
- OMMetrics omMetrics) {
+ OMMetrics omMetrics, AuditLogger auditLogger,
+ Map<String, String> auditMap) {
+ auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
+ exception, getOmRequest().getUserInfo()));
Review comment:
@ChenSammi Thank you for working on this. To address this situation, how
about this idea:
1. Let's not call the auditlog() in the first line of the current method.
2. We can do something like this:
` if (operationResult) {
LOG.debug("Add acl: {} to path: {} success!", getAcls(), getPath());
} else {
omMetrics.incNumBucketUpdateFails();
if (exception == null) {
LOG.error("Add acl {} to path {} failed, because acl already exist",
getAcls(), getPath());
} else {
LOG.error("Add acl {} to path {} failed!", getAcls(), getPath(),
exception);
exception = null; //
}
}
auditLog(auditLogger, buildAuditMessage(OMAction.ADD_ACL, auditMap,
exception, getOmRequest().getUserInfo()));
}`
This will ensure this covers the edge case mentioned by @adoroszlai and we
won't have to modify the Audit Log base framework since this scenario is for a
very limited number of occurrence in auditing ACL ops only.
What do you think?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
