Hexiaoqiao commented on code in PR #6001:
URL: https://github.com/apache/hadoop/pull/6001#discussion_r1316664713
##########
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:
> if we remove this lock, then, incrSharedCount() method is running in
compareAndSet pattern, so chances are very likely that it will try multiple
times before updating the value to ZK. So with this lock, all these challenges
can be addressed.
I don't think it could decrease retry chances where hold lock here, because
the znode could be updated by different processes, such as DFSRouter, also if
hold lock but retry multiple times, the performance will be impacted
tremendously.
> If seqNumBatchSize is set to an optimum number, this lock will be acquired
just to increment an integer number (most of time ) that should not impact the
performance. This will take time only when it interacts with ZK and it will
interact with ZK only when already reserved/generated seqNumber gets exhausted.
Totally agree, however we give batch size is 1 by default. even through tune
this configuration to one bigger number, it could not resolve the above issue.
Thanks @vikaskr22 again. JFYI.
--
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]
