joseluisll commented on code in PR #8688:
URL: https://github.com/apache/hadoop/pull/8688#discussion_r4510498437
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -96,6 +98,10 @@ public abstract class
ZKDelegationTokenSecretManager<TokenIdent extends Abstract
public static final String ZK_DTSM_TOKEN_WATCHER_ENABLED = ZK_CONF_PREFIX
+ "token.watcher.enabled";
public static final boolean ZK_DTSM_TOKEN_WATCHER_ENABLED_DEFAULT = true;
+ /** Bound on the wait for the initial cache load; 0 waits indefinitely. */
Review Comment:
**Ask (nit):** the timeout applies to each cache separately, key cache then
token cache, so startup can wait up to twice the configured value. Please say
"per cache" in the Javadoc.
A negative value also quietly means "no limit". A
`Preconditions.checkArgument(cacheInitTimeoutMs >= 0, ...)` in the constructor
would catch typos.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -290,8 +318,13 @@ public void startThreads() throws IOException {
try {
keyCache = CuratorCache.bridgeBuilder(zkClient, ZK_DTSM_MASTER_KEY_ROOT)
.build();
+ CountDownLatch keyCacheInitialized = new CountDownLatch(1);
CuratorCacheListener keyCacheListener = CuratorCacheListener.builder()
.forCreatesAndChanges((oldNode, node) -> {
+ if (ZK_DTSM_MASTER_KEY_ROOT.equals(node.getPath())) {
+ // The root node itself is a container, not a key.
+ return;
+ }
try {
processKeyAddOrUpdate(node.getData());
Review Comment:
**Ask (nit):** please include the path in the two `LOG.error` calls below
(here at line 331 and in the token listener at line 362), and in the token
delete listener. For example:
```java
LOG.error("Error while processing Curator keyCacheListener NODE_CREATED /
NODE_CHANGED event for {}", node.getPath());
```
**Why:** before this PR, `loadFromZKCache` logged `Ignoring node {path}`.
Now these lines are the only sign of a bad znode during the first load, and
they don't say which one. Curator's own `Listener (...) threw an exception` log
doesn't name it either.
##########
hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/security/token/delegation/ZKDelegationTokenSecretManager.java:
##########
@@ -290,8 +318,13 @@ public void startThreads() throws IOException {
try {
keyCache = CuratorCache.bridgeBuilder(zkClient, ZK_DTSM_MASTER_KEY_ROOT)
.build();
+ CountDownLatch keyCacheInitialized = new CountDownLatch(1);
CuratorCacheListener keyCacheListener = CuratorCacheListener.builder()
.forCreatesAndChanges((oldNode, node) -> {
+ if (ZK_DTSM_MASTER_KEY_ROOT.equals(node.getPath())) {
Review Comment:
**Ask:** please add a regression test for roots that already carry Curator's
default payload. The description calls the root skip "the actual fix", but
every test creates fresh, empty roots.
I tried this one locally on a9b7b35b46e. It passes. With the three root
guards removed it fails, because the key listener hits `EOFException` and the
token listener hits `Unknown version of delegation token 49`. All the imports
are already in the test file.
```java
@Test
public void testRootPayloadNotParsed() throws Exception {
String ns = "/testPath/ZKDTSMRoot";
byte[] payload = "127.0.0.1".getBytes(StandardCharsets.UTF_8);
try (CuratorFramework zk = CuratorFrameworkFactory.newClient(
zkServer.getConnectString(), new ExponentialBackoffRetry(1000, 3))) {
zk.start();
zk.create().creatingParentsIfNeeded().forPath(ns +
"/ZKDTSMMasterKeyRoot", payload);
zk.create().creatingParentsIfNeeded().forPath(ns + "/ZKDTSMTokensRoot",
payload);
}
GenericTestUtils.LogCapturer logs =
GenericTestUtils.LogCapturer.captureLogs(
LoggerFactory.getLogger(ZKDelegationTokenSecretManager.class));
DelegationTokenManager tm = new DelegationTokenManager(
getSecretConf(zkServer.getConnectString()), new Text("bla"));
try {
tm.init();
assertFalse(logs.getOutput().contains("Error while processing Curator"));
} finally {
tm.destroy();
logs.stopCapturing();
}
}
```
--
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]
