Hexiaoqiao commented on code in PR #6001:
URL: https://github.com/apache/hadoop/pull/6001#discussion_r1314280986
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -520,24 +525,32 @@ protected int incrementDelegationTokenSeqNum() {
// The secret manager will keep a local range of seq num which won't be
// seen by peers, so only when the range is exhausted it will ask zk for
// another range again
- if (currentSeqNum >= currentMaxSeqNum) {
- try {
- // after a successful batch request, we can get the range starting
point
- currentSeqNum = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
- currentMaxSeqNum = currentSeqNum + seqNumBatchSize;
- LOG.info("Fetched new range of seq num, from {} to {} ",
- currentSeqNum+1, currentMaxSeqNum);
- } catch (InterruptedException e) {
- // The ExpirationThread is just finishing.. so dont do anything..
- LOG.debug(
- "Thread interrupted while performing token counter increment", e);
- Thread.currentThread().interrupt();
- } catch (Exception e) {
- throw new RuntimeException("Could not increment shared counter !!", e);
+ try{
+ this.currentSeqNumLock.lock();
Review Comment:
I am concerned if it is proper here. Because there are some IO operations
(request to zk here at L533 and log print atL535) inside the
`currentSeqNumLock`, it could decrease performance significantly when meet some
network latency or disk busy.
I think it is enough to protect `currentSeqNum` and `currentMaxSeqNum`,
right? If that, is it possible to define them as AtomicInteger?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -520,24 +525,32 @@ protected int incrementDelegationTokenSeqNum() {
// The secret manager will keep a local range of seq num which won't be
// seen by peers, so only when the range is exhausted it will ask zk for
// another range again
- if (currentSeqNum >= currentMaxSeqNum) {
- try {
- // after a successful batch request, we can get the range starting
point
- currentSeqNum = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
- currentMaxSeqNum = currentSeqNum + seqNumBatchSize;
- LOG.info("Fetched new range of seq num, from {} to {} ",
- currentSeqNum+1, currentMaxSeqNum);
- } catch (InterruptedException e) {
- // The ExpirationThread is just finishing.. so dont do anything..
- LOG.debug(
- "Thread interrupted while performing token counter increment", e);
- Thread.currentThread().interrupt();
- } catch (Exception e) {
- throw new RuntimeException("Could not increment shared counter !!", e);
+ try{
+ this.currentSeqNumLock.lock();
+ if (currentSeqNum >= currentMaxSeqNum) {
+ try {
+ // after a successful batch request, we can get the range starting
point
+ currentSeqNum = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
+ currentMaxSeqNum = currentSeqNum + seqNumBatchSize;
+ LOG.info("Fetched new range of seq num, from {} to {} ",
+ currentSeqNum+1, currentMaxSeqNum);
+ } catch (InterruptedException e) {
+ // The ExpirationThread is just finishing.. so dont do anything..
+ LOG.debug(
+ "Thread interrupted while performing token counter
increment", e);
+ Thread.currentThread().interrupt();
+ } catch (Exception e) {
+ throw new RuntimeException("Could not increment shared counter !!",
e);
+ }
}
+
+ return ++currentSeqNum;
+
+ }finally{
Review Comment:
code style:
1. extra blank.
2. leave a blank space before or after brace.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -520,24 +525,32 @@ protected int incrementDelegationTokenSeqNum() {
// The secret manager will keep a local range of seq num which won't be
// seen by peers, so only when the range is exhausted it will ask zk for
// another range again
- if (currentSeqNum >= currentMaxSeqNum) {
- try {
- // after a successful batch request, we can get the range starting
point
- currentSeqNum = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
- currentMaxSeqNum = currentSeqNum + seqNumBatchSize;
- LOG.info("Fetched new range of seq num, from {} to {} ",
- currentSeqNum+1, currentMaxSeqNum);
- } catch (InterruptedException e) {
- // The ExpirationThread is just finishing.. so dont do anything..
- LOG.debug(
- "Thread interrupted while performing token counter increment", e);
- Thread.currentThread().interrupt();
- } catch (Exception e) {
- throw new RuntimeException("Could not increment shared counter !!", e);
+ try{
Review Comment:
code style here.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -149,6 +150,9 @@ protected static CuratorFramework getCurator() {
private int currentSeqNum;
private int currentMaxSeqNum;
+ private final ReentrantLock currentSeqNumLock;
+
+
Review Comment:
Extra blank space here.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -520,24 +525,32 @@ protected int incrementDelegationTokenSeqNum() {
// The secret manager will keep a local range of seq num which won't be
// seen by peers, so only when the range is exhausted it will ask zk for
// another range again
- if (currentSeqNum >= currentMaxSeqNum) {
- try {
- // after a successful batch request, we can get the range starting
point
- currentSeqNum = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
- currentMaxSeqNum = currentSeqNum + seqNumBatchSize;
- LOG.info("Fetched new range of seq num, from {} to {} ",
- currentSeqNum+1, currentMaxSeqNum);
- } catch (InterruptedException e) {
- // The ExpirationThread is just finishing.. so dont do anything..
- LOG.debug(
- "Thread interrupted while performing token counter increment", e);
- Thread.currentThread().interrupt();
- } catch (Exception e) {
- throw new RuntimeException("Could not increment shared counter !!", e);
+ try{
+ this.currentSeqNumLock.lock();
+ if (currentSeqNum >= currentMaxSeqNum) {
+ try {
+ // after a successful batch request, we can get the range starting
point
+ currentSeqNum = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
+ currentMaxSeqNum = currentSeqNum + seqNumBatchSize;
+ LOG.info("Fetched new range of seq num, from {} to {} ",
+ currentSeqNum+1, currentMaxSeqNum);
+ } catch (InterruptedException e) {
+ // The ExpirationThread is just finishing.. so dont do anything..
+ LOG.debug(
+ "Thread interrupted while performing token counter
increment", e);
+ Thread.currentThread().interrupt();
+ } catch (Exception e) {
+ throw new RuntimeException("Could not increment shared counter !!",
e);
+ }
}
+
+ return ++currentSeqNum;
+
+ }finally{
+ this.currentSeqNumLock.unlock();
}
- return ++currentSeqNum;
+
Review Comment:
remove extra blank here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
