You are viewing a plain text version of this content. The canonical link for it is here.
Posted to common-issues@hadoop.apache.org by "Bharat Viswanadham (JIRA)" <ji...@apache.org> on 2018/01/09 00:44:01 UTC
[jira] [Comment Edited] (HADOOP-9747) Reduce unnecessary UGI
synchronization
[ https://issues.apache.org/jira/browse/HADOOP-9747?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16317432#comment-16317432 ]
Bharat Viswanadham edited comment on HADOOP-9747 at 1/9/18 12:43 AM:
---------------------------------------------------------------------
{quote}1. System.setProperty(KRB5CCNAME) is not being set, previously this is being set in the case of IBM_JAVA{quote}
Not required. See above comments for justification.
{quote}
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.{quote}
Done
{quote}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.{quote}
Done
{quote}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?{quote}
Not addressed this, Left as it is. If keytab is there it will re-login, otherwise it will be a no-op. As, this change it will not break anything, left as it is.
{quote}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{quote}
Done
Minor Nits
{quote}· 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.{quote}
Addressed all the above minot Nits.
was (Author: bharatviswa):
Thanks for a great patch, this really simplifies UGI code. I have a few comments.
{quote}1. System.setProperty(KRB5CCNAME) is not being set, previously this is being set in the case of IBM_JAVA{quote}
Not required. See above comments for justification.
{quote}
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.{quote}
Done
{quote}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.{quote}
Done
{quote}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?{quote}
Not addressed this, Left as it is. If keytab is there it will re-login, otherwise it will be a no-op. As, this change it will not break anything, left as it is.
{quote}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{quote}
Done
Minor Nits
{quote}· 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.{quote}
Addressed all the above minot Nits.
> 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-trunk.02.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: common-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: common-issues-help@hadoop.apache.org