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
--
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]
