vikaskr22 commented on code in PR #6001:
URL: https://github.com/apache/hadoop/pull/6001#discussion_r1315712962
##########
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:
Thanks @Hexiaoqiao for your input.
Here I am trying to address one race condition. This method uses one
parameter, seqNumBatchSize. Default is one, with default behaviour it will
always hit ZK. But intention of this parameter is to generate the seqNumber in
batch to avoid multiple trips to ZK.
So if the value of this param is set to 500, and just assume it has reached
the limit and it's time to regenerate next 500 seqNumber, then it should be not
be done by multiple threads. So this regeneration must be made thread safe.
That's why, I have acquired this lock.
Now, coming to performance point, 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.
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.
Please provide your feedback. Thanks.
--
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]
