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

Bharat Viswanadham commented on HADOOP-9747:
--------------------------------------------

Hi [~daryn]
Thanks for a great patch, this really simplifies UGI code. I have a few 
comments.
1. System.setProperty(KRB5CCNAME) is not being set, previously this is being 
set in the case of IBM_JAVA

2. getLoginUser is no longer Synchronized. If multiple threads call this in 
parallel, multiple loginUser ugi’s will be created and they could potentially 
spin up a new thread in spawnAutoRenewalThreadForUserCreds. I would suggest to 
synchronize it for the case when loginUser is null.

3. In loginUserFromSubject method, when subject passed is null the same 
situation will occur, it could spin multiple threads for renewal. Probably, we 
don’t need to support null subject in this API, because null subject use case 
is already handled by getLoginUser.

4. As you have noted in the comments that getKeyTabEntry is not very reliable 
for external subjects. I was wondering whether we really need it. Can we get 
away by saying that it’s user’s responsibility to renew external subjects?

5. Following 3 methods perform login and update the static loginUser. It might 
make sense to add documentation that these update the global loginUser.
getLoginUser, loginUserFromSubject and loginUserFromKeytab
 
*Minor Nits*
·         getSubjectLoginLock, does not actually getLock, can we change this 
method name getSubectPrivateCredentials.
·         In hasKerberosCredential, it might make sense to return false for 
null user, otherwise we will get an NPE.
·         Can we add assert hasSubjectLoginLock() in getTgt()?
·         In unprotectedLoginUserFromSubject we should change the local 
variable name instead of overloading loginUser, only for better readability.
 
Thank You [~xyao] [~jnp] for cumulative review on the patch.

> Reduce unnecessary UGI synchronization
> --------------------------------------
>
>                 Key: HADOOP-9747
>                 URL: https://issues.apache.org/jira/browse/HADOOP-9747
>             Project: Hadoop Common
>          Issue Type: Bug
>          Components: security
>    Affects Versions: 0.23.0, 2.0.0-alpha, 3.0.0-alpha1
>            Reporter: Daryn Sharp
>            Assignee: Daryn Sharp
>            Priority: Critical
>         Attachments: HADOOP-9747-trunk.01.patch, 
> HADOOP-9747.2.branch-2.patch, HADOOP-9747.2.trunk.patch, 
> HADOOP-9747.branch-2.patch, HADOOP-9747.trunk.patch
>
>
> Jstacks of heavily loaded NNs show up to dozens of threads blocking in the 
> UGI.



--
This message was sent by Atlassian JIRA
(v6.4.14#64029)

---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]

Reply via email to