[
https://issues.apache.org/jira/browse/HADOOP-20045?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18136997#comment-18136997
]
ASF GitHub Bot commented on HADOOP-20045:
-----------------------------------------
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.
> Avoid global write-lock contention during ZooKeeper delegation token creation
> and add a KMS HTTP benchmark
> ----------------------------------------------------------------------------------------------------------
>
> Key: HADOOP-20045
> URL: https://issues.apache.org/jira/browse/HADOOP-20045
> Project: Hadoop Common
> Issue Type: Improvement
> Components: common, kms
> Reporter: Wei-Chiu Chuang
> Assignee: Wei-Chiu Chuang
> Priority: Major
> Labels: pull-request-available
> Attachments: kms-http-latency.png, kms-http-throughput.png,
> zk-token-creation-lock-flow.png
>
>
> h3. Problem
> KMS requests authenticated with delegation tokens can stall behind a request
> issuing a new token.
> HADOOP-18851 replaced synchronization in AbstractDelegationTokenSecretManager
> with a read-write lock, allowing concurrent verification. However,
> createPassword() still holds the global write lock while allocating a
> sequence-number range and persisting the token in ZooKeeper. A slow ZooKeeper
> request therefore blocks unrelated requests that need the read lock to verify
> cached tokens.
> A thread dump from an affected KMS instance shows the lock holder waiting in:
> {noformat}
> AbstractDelegationTokenSecretManager.createPassword()
> -> ZKDelegationTokenSecretManager.storeToken()
> -> ZKDelegationTokenSecretManager.addOrUpdateToken()
> -> ZooKeeper.create()
> -> ClientCnxn.submitRequest()
> -> Object.wait()
> {noformat}
> KMS uses DelegationTokenManager.ZKSecretManager, which extends the common
> ZKDelegationTokenSecretManager. The contention fix therefore belongs in the
> common secret-manager implementation; the Router implementation is an
> additional consumer.
> h3. Proposed change
> Separate the ZooKeeper creation path into three stages:
> # Allocate a sequence number under a dedicated allocator lock, reserving a
> new range when necessary.
> # Initialize the identifier and compute its password under a short global
> write-lock section.
> # Persist the prepared token synchronously outside the global lock, returning
> success only after persistence completes.
> Preserve existing storage-hook locking for non-ZooKeeper implementations. An
> interrupted sequence-range refill must abort issuance rather than fall
> through and consume an unreserved number.
> The HDFS Router ZooKeeper subclass also needs an adjustment: creation must
> not insert a speculative cache entry before persistence or unconditionally
> insert one afterward. Existing watcher, cache-miss, and rebuild paths
> populate newly created tokens. This avoids a creator restoring stale local
> state after a concurrent cancellation.
> !zk-token-creation-lock-flow.png|width=1000!
> This change addresses creation-induced contention. Renewal, cancellation, key
> rotation, and existing distributed-cache ordering races remain separate work.
> It does not claim immediate cross-manager revocation or complete distributed
> consistency.
> h3. Correctness validation
> The change adds tests covering:
> * Verification of an existing cached token while creation is paused during
> allocation or persistence.
> * Unique sequence numbers during concurrent issuance across two managers.
> * Persistence failure without speculative cache publication.
> * Interrupted sequence-range refill.
> * Key rotation between token preparation and persistence.
> * Router creation and immediate verification with watchers disabled.
> The two blocking regression tests time out against the original code and pass
> with the fix. The selected common and Router validation comprised 43 passing
> tests. The HTTP benchmark's scheduler and latency-accounting tests and the
> existing KMS authentication-filter test also passed.
> h3. End-to-end KMS HTTP benchmark
> Add an opt-in benchmark in hadoop-kms and a comparison runner using:
> * One KMS server JVM, one standalone ZooKeeper JVM, and a separate
> load-generator JVM.
> * 32 concurrent HTTP requests; no concurrency sweep.
> * Sequence-number batch size 100, unchanged between versions.
> * No injected latency or sleep in the request/storage path.
> * Verify:create request mixes of 100:0, 99:1, 95:5, and 90:10.
> * Five seconds of warmup and fifteen seconds of measurement per case, with
> three repetitions.
> * Fresh server processes/data per case and alternating original/fixed pair
> order across repetitions.
> Verification traffic consists of metadata GET requests authenticated using
> existing delegation tokens. Creation traffic uses GETDELEGATIONTOKEN with
> simple primary authentication. Cookie reuse, redirects, and automatic retries
> are disabled to preserve the intended authentication paths and request mix.
> Before measuring, the harness checks real delegation-token authentication,
> rejection of an invalid token and an unauthenticated request, and the actual
> ZKSecretManager implementation. Diagnostic counters are disabled before load.
> Loaded secret-manager class fingerprints are checked against the selected
> original/fixed common jars in every case.
> Submitted mixes are enforced in shuffled blocks of 100. Block-tail
> submissions and drained completions are recorded separately; post-cutoff
> completions do not inflate measurement-window goodput. Token-node growth must
> equal the number of completed create operations. The harness records
> throughput, errors, per-operation latency, JVM CPU/GC snapshots, and
> ZooKeeper statistics, including real fsync activity.
> h3. Results
> Baseline: Apache trunk commit {{a2d306c65a84bdf11a6eb5251cfff1bfe110546e}}.
> Both versions use the same KMS/client benchmark and dependencies, differing
> in the selected common secret-manager implementation. JDK 17.0.15 was used.
> Median successful total HTTP requests per second across three repetitions:
> || Verify:create || Original requests/s || Fixed requests/s || Fixed/original
> ||
> | 100:0 | 30,811 | 30,150 | 0.98x |
> | 99:1 | 17,672 | 26,885 | 1.52x |
> | 95:5 | 5,349 | 23,949 | 4.48x |
> | 90:10 | 3,103 | 23,174 | 7.47x |
> !kms-http-throughput.png|width=1000!
> All 24 cases completed with zero HTTP or response-validation errors and
> passed loaded-artifact, submitted-ratio, and token-count checks. Whiskers in
> the charts show observed min-max ranges, not confidence intervals.
> Median per-run p99 latency in milliseconds:
> || Verify:create || Original verify p99 || Fixed verify p99 || Original
> create p99 || Fixed create p99 ||
> | 100:0 | 4.14 | 4.48 | N/A | N/A |
> | 99:1 | 5.31 | 4.18 | 7.08 | 18.97 |
> | 95:5 | 14.22 | 4.31 | 17.18 | 21.38 |
> | 90:10 | 21.81 | 3.39 | 22.47 | 19.35 |
> !kms-http-latency.png|width=1100!
> The 100:0 control showed variability, including one lower fixed-build
> observation; its median was 2.1 percent lower with the fix. Three short
> repetitions do not establish statistical significance for small differences.
> Creation p99 increased at 99:1 and 95:5 under the higher achieved load. These
> are closed-loop capacity comparisons, not matched-arrival-rate latency
> comparisons, and the results do not establish uniformly improved creation
> latency.
> The workload measures authenticated metadata requests with simple
> authentication for issuance and local standalone ZooKeeper. It does not
> establish production throughput for Kerberos, TLS, encryption operations, or
> a ZooKeeper quorum. No performance threshold is imposed in normal CI.
> h3. Reproduction
> The HTTP benchmark is opt-in; ordinary test runs execute only the lightweight
> scheduler and latency-accounting checks. Defaults match the matrix above.
> {noformat}
> mvn -pl hadoop-common-project/hadoop-kms \
> -Dtest=TestKMSHttpBenchmark -Dkms.benchmark=true \
> -Dmaven.test.failure.ignore=false test
> {noformat}
> For controlled comparison, build original and fixed hadoop-common jars with
> otherwise identical source/dependency versions, then run:
> {noformat}
> python3 dev-support/benchmarks/kms-http-benchmark.py \
> --baseline-common /path/to/original/hadoop-common.jar \
> --fixed-common /path/to/fixed/hadoop-common.jar \
> --output /path/to/fresh/results-directory
> {noformat}
> The runner saves incremental comparison JSON and per-case commands, logs,
> configuration, and metrics. Maven 3.9.15 or newer and JDK 17 are required for
> this trunk checkout. No native profile is required.
> Related: [HADOOP-18851|https://issues.apache.org/jira/browse/HADOOP-18851].
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
