hadoop-mapreduce-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Hemanth Yamijala (JIRA)" <j...@apache.org>
Subject [jira] Commented: (MAPREDUCE-1543) Log messages of JobACLsManager should use security logging of HADOOP-6586
Date Tue, 16 Mar 2010 07:44:27 GMT

    [ https://issues.apache.org/jira/browse/MAPREDUCE-1543?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12845757#action_12845757
] 

Hemanth Yamijala commented on MAPREDUCE-1543:
---------------------------------------------

Some review comments on the patch uploaded (mapreduce-1543-y20s.patch):

Most of the comments are minor adjustments to the log API to make it a little of cleaner..
Overall the approach in the patch seems clean to me.

- I think AuditLogManager can be pulled into a separate class file.
- Keeping the list of keys as javadoc comments seems like a maintenance overhead. I would
instead suggest that the Keys be defined as an Enum. Then it will be easy to query the Enum
names as an API. Doing this will also prevent the duplication of the key names in the logSuccess
and logFailure APIs.
- I am not sure 'agent' is conveying the right meaning. Seems like 'user' or 'userid' will
be more clear. What is the equivalent in DFS ? Also, ip should be 'remote-ip'.
- Reason seems confusing. Can we keep it generic - maybe as a string ? For e.g. in JobInProgress,
where we check the conf user is the same as the submitting user, and fails, 'permission denied'
is not giving a proper meaning. We can expand and say what exactly failed. In fact, maybe
we can change the name of the tag 'Reason' to 'AdditionalInfo' or something like that so it
is more general. Then, in QueueManager, I think it makes sense to log the jobid, when it is
available (note there are cases when it is not available), maybe as additional Info. This
way we can do things like grep by job-id and get a full trail.
- Don't tabs make the lines appear too long, Blanks would suffice, no ?
- Some values like 'permissions' and 'reason' (if this is a free format string) can have whitespaces.
Would it be good to surround these with quotes for easy parsing ?
- Suggest the parameter 'operation' could be of type String as well - if we are going to do
a toString anyway.
- If there's no cost to logging remote-ip, I'd recommend adding it everywhere as a mandatory
parameter.
- The comment '//for testcases' is misleading. I think what you mean is that the check for
IP being null is required as test cases may not have this defined. I'd suggest it be documented
as such.
- Why are we logging success again in JobTracker.addJob, wouldn't the same line come up in
QueueManager.checkAccess ?
- Seems like Operations "REFRESH_NODES", "QUEUE_REFRESH" and target "Jobtracker" can be static
final variables. Should we pull all such AuditLogManager related constants into some constants
file ? Like AuditLogConstants or something ?
- "REFRESH_NODES" and "QUEUE_REFRESH" seem similar operations, but they have different names,
it should be "REFRESH_NODES" and "REFRESH_QUEUES"
- JobACLsManager.checkAccess has some lines incorrectly indented.
- I don't think we need to give 'Queue:' prefix, it is understood that if it is a queue operation,
then the target will be a queue.
- Failure logging should be at WARN level - as done in the other audit log.
- Can we move the refreshAcls logging into QueueManager.refreshQueueAcls - so that failures
can be logged too.
- Can we add a few simple unit tests that test the formatting of the log message alone. i.e.
we can define a package private API in AuditLogManager - something like String getLogLine(...).
We can call these in the log*** APIs in AuditLogManager. We can also call it with various
arguments in tests and verify that the return string is proper. We can test null values, values
with whitespace etc.

> Log messages of JobACLsManager should use security logging of HADOOP-6586
> -------------------------------------------------------------------------
>
>                 Key: MAPREDUCE-1543
>                 URL: https://issues.apache.org/jira/browse/MAPREDUCE-1543
>             Project: Hadoop Map/Reduce
>          Issue Type: Bug
>          Components: security
>            Reporter: Vinod K V
>            Assignee: Amar Kamat
>             Fix For: 0.22.0
>
>         Attachments: mapreduce-1543-y20s.patch
>
>
> {{JobACLsManager}} added in MAPREDUCE-1307 logs the successes and failures w.r.t job-level
authorization in the corresponding Daemons' logs. The log messages should instead use security
logging of HADOOP-6586.

-- 
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.


Mime
View raw message