jojochuang commented on code in PR #8888:
URL: https://github.com/apache/hadoop/pull/8888#discussion_r4451306195
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -501,27 +504,39 @@ private int incrSharedCount(SharedCount sharedCount, int
batchSize)
@Override
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);
+ synchronized (sequenceNumberLock) {
+ // A range belongs exclusively to this manager. Never consume a number
+ // beyond the range unless its replacement was successfully reserved.
+ if (currentSeqNum >= currentMaxSeqNum) {
+ try {
+ int start = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
Review Comment:
Agreed. Extracted reserveTokenSequenceRange() and called it from both
startup and runtime refill in f51e54bf236. The helper reserves the range before
updating local allocator state. Exception handling stays at the call sites:
startup retains its IOException behavior, and runtime refill restores the
interrupt flag and throws IllegalStateException on interruption. Re-ran
TestDelegationToken, TestZKDelegationTokenCreation, and
TestZKDelegationTokenSecretManager: 41 passed, zero failures/errors, and one
opt-in benchmark skipped.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -266,10 +267,12 @@ public void startThreads() throws IOException {
}
// the first batch range should be allocated during this starting window
// by calling the incrSharedCount
- currentSeqNum = incrSharedCount(delTokSeqCounter, seqNumBatchSize);
- currentMaxSeqNum = currentSeqNum + seqNumBatchSize;
- LOG.info("Fetched initial range of seq num, from {} to {} ",
- currentSeqNum+1, currentMaxSeqNum);
+ synchronized (sequenceNumberLock) {
Review Comment:
Correct, normal initialization is single-threaded. I retained this
uncontended lock so all accesses to the local allocator fields follow the same
locking discipline, including the shared reserveTokenSequenceRange() helper
extracted in f51e54bf236. The helper now documents that callers must hold
sequenceNumberLock, including during initialization. This is the dedicated
allocator lock, not apiLock, so ZooKeeper range reservation does not block
token verification.
--
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]
