[
https://issues.apache.org/jira/browse/HADOOP-13396?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15414529#comment-15414529
]
Wei-Chiu Chuang commented on HADOOP-13396:
------------------------------------------
Hi [~xiaochen] thanks for the updated patch. Overall looks good to me. I have a
few comments, and most are cosmetic:
* {code:title=KMSAudit.JsonKMSAuditLogger#logAuditEvent}
LOG.error("Exception caught when logging {}", event, e);
{code}
This won’t print the exception nor its stack trace. The similar kind of change
is needed for handling other exceptions.
* In {{KMSAuditLogger.AuditEvent#toString}}
you can actually do: {{sb.append(X).append(Y).append(Z);}}
* {{TestKMSAuditJson}}
please import classes explicitly instead of wildcard import.
{code}
import static
org.apache.hadoop.crypto.key.kms.server.KMSAudit.JsonKMSAuditLogger.*;
{code}
* KMSAudit#error:
I noticed the url parameter of this method is not used. Would it make sense to
append the url to extraMsg?
* KMSAudit#initializeAuditLoggers
I suppose the ‘continue’ is redundant?
{code}
for (String l : loggers) {
if (l.equals(SimpleKMSAuditLogger.TYPE)) {
auditLoggers.add(new SimpleKMSAuditLogger());
} else if (l.equals(JsonKMSAuditLogger.TYPE)) {
auditLoggers.add(new JsonKMSAuditLogger());
continue;
} else {
LOG.warn("Ignored unknown audit logger type {}", l);
}
{code}
* KMSAudit#op:
It does not make sense to me that the code throws a RuntimeException when an
ExecutionException is thrown. It transforms a checked exception to an unchecked
one, and if the exception is thrown, the entire process is terminated if I
understand it correctly.
> Add json format audit logging to KMS
> ------------------------------------
>
> Key: HADOOP-13396
> URL: https://issues.apache.org/jira/browse/HADOOP-13396
> Project: Hadoop Common
> Issue Type: New Feature
> Components: kms
> Reporter: Xiao Chen
> Assignee: Xiao Chen
> Attachments: HADOOP-13396.01.patch, HADOOP-13396.02.patch,
> HADOOP-13396.03.patch, HADOOP-13396.04.patch
>
>
> Currently, KMS audit log is using log4j, to write a text format log.
> We should refactor this, so that people can easily add new format audit logs.
> The current text format log should be the default, and all of its behavior
> should remain compatible.
> A json format log extension is added using the refactored API, and being
> turned off by default.
--
This message was sent by Atlassian JIRA
(v6.3.4#6332)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
