-
Notifications
You must be signed in to change notification settings - Fork 8.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HADOOP-18851: Performance improvement for DelegationTokenSecretManager. #6001
Conversation
@jojochuang , as per our discussion on JIRA, this is the PR. Please review and provide your inputs. Thanks. |
💔 -1 overall
This message was automatically generated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vikaskr22 Thanks for your works. leave some comments inline. FYI.
...rc/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java
Outdated
Show resolved
Hide resolved
...rc/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java
Show resolved
Hide resolved
|
||
return ++currentSeqNum; | ||
|
||
}finally{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
code style:
- extra blank.
- leave a blank space before or after brace.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
incorporated. Thanks.
} | ||
|
||
return ++currentSeqNum; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove extra blank here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incorporated. Thanks.
💔 -1 overall
This message was automatically generated. |
c49127c
to
827b881
Compare
💔 -1 overall
This message was automatically generated. |
💔 -1 overall
This message was automatically generated. |
...n/java/org/apache/hadoop/security/token/delegation/AbstractDelegationTokenSecretManager.java
Show resolved
Hide resolved
💔 -1 overall
This message was automatically generated. |
Hi @Hexiaoqiao , @jojochuang , I have incorporated all the review comments. Now CI pipeline is failing due to lack of test cases. Please let me know if this sufficient , if not, please suggest further. Thanks. |
It is OK for me. Please fix the checkstyle as Yetus reported before approve and checkin. Thanks. |
💔 -1 overall
This message was automatically generated. |
@Hexiaoqiao , @jojochuang , are we good to merge this ? I can see one approval. If yes, then can anyone of you please do the needful. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. +1.
Committed to trunk. Thanks @vikaskr22 for your works and @jojochuang for your review! |
…r. (apache#6001). Contributed by Vikas Kumar. Signed-off-by: Wei-Chiu Chuang <[email protected]> Signed-off-by: He Xiaoqiao <[email protected]>
…etManager. (apache#6001). Contributed by Vikas Kumar." This reverts commit e283375.
…etManager. (apache#6001). Contributed by Vikas Kumar." This reverts commit e283375.
Description of PR
AbstractDelegationTokenSecretManager's method all synchronized and are blocking each other, even multiple readers threads are blocking each other. This PR is an efforts towards optimising the synchronization contexts.
For detailed description, please go through the discussion on https://issues.apache.org/jira/browse/HADOOP-18851
How was this patch tested?
Build is working fine. And this is more about a logical change like where to acquire or release a lock. It requires careful review. There is not any functional logic change.
For code changes:
LICENSE
,LICENSE-binary
,NOTICE-binary
files?