ambari-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hadoop QA (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (AMBARI-15646) Audit Log Code Cleanup & Safety
Date Thu, 07 Apr 2016 20:20:25 GMT

    [ https://issues.apache.org/jira/browse/AMBARI-15646?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15230982#comment-15230982
] 

Hadoop QA commented on AMBARI-15646:
------------------------------------

{color:red}-1 overall{color}.  Here are the results of testing the latest attachment 
  http://issues.apache.org/jira/secure/attachment/12797529/AMBARI-15646.patch
  against trunk revision .

    {color:green}+1 @author{color}.  The patch does not contain any @author tags.

    {color:green}+1 tests included{color}.  The patch appears to include 10 new or modified
test files.

    {color:green}+1 javac{color}.  The applied patch does not increase the total number of
javac compiler warnings.

    {color:green}+1 release audit{color}.  The applied patch does not increase the total number
of release audit warnings.

    {color:red}-1 core tests{color}.  The patch failed these unit tests in ambari-server:

                  org.apache.ambari.server.state.alerts.AlertReceivedListenerTest

Test results: https://builds.apache.org/job/Ambari-trunk-test-patch/6292//testReport/
Console output: https://builds.apache.org/job/Ambari-trunk-test-patch/6292//console

This message is automatically generated.

> Audit Log Code Cleanup & Safety
> -------------------------------
>
>                 Key: AMBARI-15646
>                 URL: https://issues.apache.org/jira/browse/AMBARI-15646
>             Project: Ambari
>          Issue Type: Bug
>          Components: ambari-server
>            Reporter: Daniel Gergely
>            Assignee: Daniel Gergely
>         Attachments: AMBARI-15646.patch
>
>
> As a follow-up to AMBARI-15241, there are concerns brought up in review which should
be addressed but didn't need to hold up the feature being committed. These can be further
broken out to into separate Jiras if needed:
> - When initializing a ThreadLocal, you can specify an initial value. This code is unnecessary:
> {code}
> private ThreadLocal<DateFormat> dateFormatThreadLocal = new ThreadLocal<>();
>     if(dateFormatThreadLocal.get() == null) {
>       //2016-03-11T10:42:36.376Z
>       dateFormatThreadLocal.set(new SimpleDateFormat("yyyy-MM-dd'T'HH:mm:ss.SSSX"));
>     }
> {code}
> - There are no tests for a majority of events and event creators.
> - Using a multibinder is fine to be able to inject a {{Set<Foo>}}, but it's not
clear to developers adding code that this is what must be done. 
> -- We either need to document the super interface to make it clear how to have new creators
bound
> -- Or annotate creators with an annotation which then be automatically picked up by the
{{AuditLoggerModule}} and bound without the need to explicitly define each creator.
> - {code}
>     // binding for audit event creators
>     Multibinder<RequestAuditEventCreator> auditLogEventCreatorBinder = Multibinder.newSetBinder(binder(),
RequestAuditEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(DefaultEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(ComponentEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(ServiceEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(UnauthorizedEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(ConfigurationChangeEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(UserEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(GroupEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(MemberEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(PrivilegeEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(BlueprintExportEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(ServiceConfigDownloadEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(BlueprintEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(ViewInstanceEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(ViewPrivilegeEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(RepositoryEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(RepositoryVersionEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(AlertGroupEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(AlertTargetEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(HostEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(UpgradeEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(UpgradeItemEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(RecommendationIgnoreEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(ValidationIgnoreEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(CredentialEventCreator.class);
>     auditLogEventCreatorBinder.addBinding().to(RequestEventCreator.class);
>     bind(RequestAuditLogger.class).to(RequestAuditLoggerImpl.class);
> {code}
> - Event creators have nested invocations which is not only hard to read, but can potentially
cause NPE's; it's a dangerous practice. As an example:
> {code:title=AlertGroupEventCreator}
> String.valueOf(request.getBody().getNamedPropertySets().iterator().next().getProperties().get(PropertyHelper.getPropertyId("AlertGroup",
"name")));
> {code}
> -- Additionally, this references properties by building them, instead of by their registration
in the property provider. If the property name ever changed, this could easily be missed.
> - Some of the {{auditLog}} methods check to ensure that the logger is enabled first.
This is very good, as building objects which won't be logged is a waste and potential performance
problem. However, not all of them do. All {{auditLog}} methods should check this first, and
return if not enabled. You can do this using AOP or just brute-force every method.
> {code}
>   private void auditLog(HostRoleCommandEntity commandEntity, Long requestId) {
>     if(!auditLogger.isEnabled()) {
>       return;
>     }
> {code}
> - The temporary status caches in {{ActionDBAccessorImpl}} are never cleared.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Mime
View raw message