[
https://issues.apache.org/jira/browse/HADOOP-5419?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=12706791#action_12706791
]
Hemanth Yamijala commented on HADOOP-5419:
------------------------------------------
Looks fine overall. Few minor comments and suggestions:
- Indentation incorrect in a few places like displayQueueAclsInfoForCurrentUser.
- Wrap to 80 characters at all places in the code.
- Have a space between the opening brace of a block and the condition.
- Message: "User <username> doesnt have access to any queue ": change doesnt to
"does not". Terminate with a stop. So: User <username> does not have access to
any queue.
- Why do we need UnixUserGroupInformation.setCurrentUser(ugi); in getUGI(). I
think we can use the conf defined in JobQueueClient to retrieve the current UGI
information since the getUGI method saves it to this conf.
- getCurrentUserQueueAclsInfo - I think a better name is
getQueueAclsForCurrentUser(). This is inline with the other APIs defined.
- versionId documentation in JobSubmissionProtocol has wrong method name.
- JobTracker.submitJob() - The log statement should be included in the
IOException as well so it can be printed to the client.
- Javadoc: Gets the Queue ACLs for a user in the @return. Similarly in the
comment in JobSubmissionProtocol.
- QueueManager.getCurrentUserQueueAclsInfo - I think at this level it can be
generic and make it QueueManager.getQueueAcls(UGI user). So tomorrow if there's
a need to give for a specific user then we can use the same API.
- In QueueAclsInfo, rather than use ArrayWritable, at other places in M/R, we
write a count of the number of items, and then write each item. The elements
are stored in a list that's created along with the object. This seems to be
better as it does not need new objects to be created each time as is being done
now.
- I think it would be better to print the ACLs in a table format like:
Queue Operations
queuesdjkhdsfksjd1 submit-job,adminster-jobs
queue2 submit-job
and so on. Also, avoid space between the operations. This will help the output
to work well with standard Unix like tools.
- Test cases: I think we should add the current user to some of the queues and
not set an arbitrary job ugi (like u1). You can look at TestQueueManager to see
how we get get the Current user's UGI.
- For qu1, why would we not check that both submit and administer jobs is
available. Also, rather than checking for substring, I would verify that the
entire string is available.
- The group can be the current group for qu5, similar to current user for other
queues
- I think there must be another test in which the current user has no access to
any queues and verify that an empty list is returned.
> Provide a way for users to find out what operations they can do on which M/R
> queues
> -----------------------------------------------------------------------------------
>
> Key: HADOOP-5419
> URL: https://issues.apache.org/jira/browse/HADOOP-5419
> Project: Hadoop Core
> Issue Type: Improvement
> Components: mapred
> Reporter: Hemanth Yamijala
> Assignee: rahul k singh
> Attachments: commands_manual.pdf, hadoop-5419-1.patch,
> hadoop-5419-1.patch, hadoop-5419-2.patch, hadoop-5419-2.patch,
> hadoop-5419.patch, hadoop-5419.patch, hadoop-5419.patch, hadoop-5419.patch
>
>
> This issue is to provide an improvement on the existing M/R framework to let
> users know which queues they have access to, and for what operations. One use
> case for this would that currently there is no easy way to know if the user
> has access to submit jobs to a queue, until it fails with an access control
> exception.
--
This message is automatically generated by JIRA.
-
You can reply to this email to add a comment to the issue online.
