[
https://issues.apache.org/jira/browse/HADOOP-19966?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18109623#comment-18109623
]
ASF GitHub Bot commented on HADOOP-19966:
-----------------------------------------
slfan1989 commented on code in PR #8688:
URL: https://github.com/apache/hadoop/pull/8688#discussion_r3889022307
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -301,9 +310,11 @@ public void startThreads() throws IOException {
}
})
.forDeletes(childData -> processKeyRemoved(childData.getPath()))
+ .forInitialized(keyCacheInitialized::countDown)
.build();
keyCache.listenable().addListener(keyCacheListener);
keyCache.start();
+ awaitCacheInitialized(keyCacheInitialized, "key");
Review Comment:
When cache initialization times out or is interrupted, `startThreads()`
throws after the cache and shared counters have already been started. The
caller does not automatically invoke `stopThreads()` after a failed
initialization, so these resources may remain active. Could the partially
started resources be closed before propagating the failure?
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -386,6 +409,20 @@ private void loadFromZKCache(final boolean isTokenCache) {
LOG.info("Loaded {} cache.", cacheName);
}
+ private void awaitCacheInitialized(CountDownLatch initialized,
+ String cacheName) throws IOException {
+ try {
+ if (!initialized.await(CACHE_INITIALIZED_TIMEOUT_SECONDS,
TimeUnit.SECONDS)) {
Review Comment:
`initialized()` is emitted only after all initial nodes have been loaded
into the cache, so the required time depends on the size of the token/key trees
and ZooKeeper latency. A fixed 10-second timeout can make an otherwise healthy
service fail to start when the cache is large or the configured ZooKeeper
timeout is longer.
Could this timeout be configurable, or derived from an existing ZooKeeper
timeout setting, with an appropriate default?
##########
hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/security/token/delegation/TestZKDelegationTokenSecretManager.java:
##########
@@ -522,6 +522,13 @@ public Boolean get() {
// The good token should be loaded on startup, and removed after expiry.
id = smNew.decodeTokenIdentifier(token);
+ final AbstractDelegationTokenIdentifier idGood = id;
+ GenericTestUtils.waitFor(new Supplier<Boolean>() {
Review Comment:
The production change is intended to guarantee that the cache is fully
loaded before `tm.init()` returns. Adding an eventual 5-second wait here
weakens that regression guarantee: the old asynchronous implementation could
also pass once its listener catches up.
Could we keep the immediate assertion after `tm.init()` ? Ideally, the test
should fail without the `awaitCacheInitialized()` production change and pass
with it.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -333,9 +349,11 @@ public void startThreads() throws IOException {
throw new UncheckedIOException(e);
}
})
+ .forInitialized(tokenCacheInitialized::countDown)
.build();
tokenCache.listenable().addListener(tokenCacheListener);
tokenCache.start();
+ awaitCacheInitialized(tokenCacheInitialized, "token");
Review Comment:
Same Line#317
> ZKDelegationTokenSecretManager may fail to load tokens and keys from
> ZooKeeper on startup
> -----------------------------------------------------------------------------------------
>
> Key: HADOOP-19966
> URL: https://issues.apache.org/jira/browse/HADOOP-19966
> Project: Hadoop Common
> Issue Type: Bug
> Reporter: Cheng Pan
> Priority: Major
> Labels: pull-request-available
>
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
