Copilot commented on code in PR #6677:
URL: https://github.com/apache/hive/pull/6677#discussion_r3720856778
##########
llap-client/src/java/org/apache/hadoop/hive/registry/impl/ZkRegistryBase.java:
##########
@@ -651,14 +652,14 @@ protected final synchronized PathChildrenCache
ensureInstancesCache(
long elapsedNs = System.nanoTime() - startTimeNs;
if (deltaNs == 0 || deltaNs <= elapsedNs) {
LOG.error("Unable to start curator PathChildrenCache", e);
- throw new IOException(e);
+ throw new ClusterNotReadyException(e);
}
LOG.warn("The cluster is not started yet (InvalidACL); will retry");
try {
Thread.sleep(Math.min(sleepTimeMs, (deltaNs - elapsedNs)/1000000L));
} catch (InterruptedException e1) {
LOG.error("Interrupted while retrying the PathChildrenCache
startup");
- throw new IOException(e1);
+ throw new ClusterNotReadyException(e1);
Review Comment:
When catching `InterruptedException`, the thread interrupt flag should be
restored (`Thread.currentThread().interrupt()`), otherwise higher-level
cancellation/shutdown logic may not observe the interrupt. Also consider
logging the exception (`LOG.error(..., e1)`) so the cause is not lost in logs.
##########
ql/src/test/org/apache/hadoop/hive/llap/TestProactiveEviction.java:
##########
@@ -0,0 +1,189 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.llap;
+
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.framework.recipes.nodes.PersistentEphemeralNode;
+import org.apache.curator.retry.RetryOneTime;
+import org.apache.curator.test.TestingServer;
+import org.apache.curator.utils.CloseableUtils;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.io.api.LlapProxy;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.hive.llap.registry.impl.LlapZookeeperRegistryImpl;
+import org.apache.hadoop.hive.registry.impl.ZkRegistryBase;
+import org.apache.hadoop.registry.client.binding.RegistryTypeUtils;
+import org.apache.hadoop.registry.client.binding.RegistryUtils;
+import org.apache.hadoop.registry.client.types.ServiceRecord;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.net.URI;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+
+import static org.junit.Assert.fail;
+
+/**
+ * Tests for {@link ProactiveEviction} focusing on the ZooKeeper-based LLAP
registry interaction
+ * with Kerberos authentication enabled.
+ *
+ * The tests use a local TestingServer (embedded ZooKeeper) and mock UGI to
simulate a secure
+ * environment without requiring a real KDC. The "llap-sasl" namespace is used
because
+ * HIVE_ZOOKEEPER_USE_KERBEROS is enabled, which is the namespace the registry
uses in production
+ * when Kerberos is active.
+ */
+public class TestProactiveEviction {
+
+ private HiveConf hiveConf = new HiveConf();
+
+ private CuratorFramework curatorFramework;
+ private TestingServer server;
+
+ private UserGroupInformation ugi;
+
+ MockedStatic<UserGroupInformation> userGroupInformationMockedStatic;
+
+ @Before
+ public void setUp() throws Exception {
+ ugi = Mockito.mock(UserGroupInformation.class);
+ userGroupInformationMockedStatic =
Mockito.mockStatic(UserGroupInformation.class);
+
userGroupInformationMockedStatic.when(UserGroupInformation::isSecurityEnabled).thenReturn(true);
+
userGroupInformationMockedStatic.when(UserGroupInformation::getCurrentUser).thenReturn(ugi);
+ Mockito.when(ugi.getShortUserName()).thenReturn("hive");
+
+ server = new TestingServer();
+ server.start();
Review Comment:
In Curator’s `TestingServer`, the default constructor typically starts the
server immediately; calling `start()` again can throw (or behave
inconsistently) depending on Curator version. Prefer using just `new
TestingServer()` without `start()`, or use the constructor variant that does
not auto-start if you need explicit control.
##########
llap-client/src/test/org/apache/hadoop/hive/llap/registry/impl/TestLlapZookeeperRegistryImpl.java:
##########
@@ -99,6 +108,66 @@ public void testRegister() throws Exception {
parseInt(attributes.get(LlapRegistryService.LLAP_DAEMON_NUM_ENABLED_EXECUTORS)));
}
+ @Test
+ public void testRetryOnInvalidACLException() throws Exception {
+ // Given
+ LlapZookeeperRegistryImpl underTest =
+ new LlapZookeeperRegistryImpl("ClientRegistryRetryTest", hiveConf);
+
+ ACLProvider aclProvider = Mockito.mock(ACLProvider.class);
+ ACL allowAll = new ACL(ZooDefs.Perms.ALL, ZooDefs.Ids.ANYONE_ID_UNSAFE);
+ Mockito.when(aclProvider.getAclForPath(Mockito.any())).
+ thenReturn(Collections.emptyList()). // causes InvalidACLException
+ thenReturn(Collections.singletonList(allowAll)); // allow all
+
+ CuratorFramework curatorFrameworkWithAclProvider = CuratorFrameworkFactory.
+ builder().
+ connectString(server.getConnectString()).
+ sessionTimeoutMs(10000).
+ retryPolicy(new RetryOneTime(1000)).
+ aclProvider(aclProvider).
+ build();
+
+ trySetMock(underTest, "zooKeeperClient", curatorFrameworkWithAclProvider);
+ underTest.start();
+
+ // When
+ ServiceInstanceSet<LlapServiceInstance> serviceInstanceSet =
+ underTest.getInstances("LLAP", 10000);
+
+ // Then
+ Collection<LlapServiceInstance> llaps = serviceInstanceSet.getAll();
+ assertEquals(0, llaps.size());
+ Mockito.verify(aclProvider,
Mockito.atLeast(4)).getAclForPath(Mockito.any());
Review Comment:
This test constructs a `CuratorFramework` but does not explicitly close it,
which can leak threads/sockets across the test suite. Close
`curatorFrameworkWithAclProvider` in a `finally` block (or via
`CloseableUtils`) after the assertions. Also, `atLeast(4)` is brittle across
Curator/ZK versions and internal call paths; to assert retry occurred, it’s
usually enough to verify it was called more than once (e.g., `atLeast(2)`) or
to assert based on observable behavior rather than an exact-ish internal call
count.
##########
llap-client/src/test/org/apache/hadoop/hive/llap/registry/impl/TestLlapZookeeperRegistryImpl.java:
##########
@@ -99,6 +108,66 @@ public void testRegister() throws Exception {
parseInt(attributes.get(LlapRegistryService.LLAP_DAEMON_NUM_ENABLED_EXECUTORS)));
}
+ @Test
+ public void testRetryOnInvalidACLException() throws Exception {
+ // Given
+ LlapZookeeperRegistryImpl underTest =
+ new LlapZookeeperRegistryImpl("ClientRegistryRetryTest", hiveConf);
+
+ ACLProvider aclProvider = Mockito.mock(ACLProvider.class);
+ ACL allowAll = new ACL(ZooDefs.Perms.ALL, ZooDefs.Ids.ANYONE_ID_UNSAFE);
+ Mockito.when(aclProvider.getAclForPath(Mockito.any())).
+ thenReturn(Collections.emptyList()). // causes InvalidACLException
+ thenReturn(Collections.singletonList(allowAll)); // allow all
+
+ CuratorFramework curatorFrameworkWithAclProvider = CuratorFrameworkFactory.
+ builder().
+ connectString(server.getConnectString()).
+ sessionTimeoutMs(10000).
+ retryPolicy(new RetryOneTime(1000)).
+ aclProvider(aclProvider).
+ build();
+
+ trySetMock(underTest, "zooKeeperClient", curatorFrameworkWithAclProvider);
+ underTest.start();
+
+ // When
+ ServiceInstanceSet<LlapServiceInstance> serviceInstanceSet =
+ underTest.getInstances("LLAP", 10000);
Review Comment:
This test constructs a `CuratorFramework` but does not explicitly close it,
which can leak threads/sockets across the test suite. Close
`curatorFrameworkWithAclProvider` in a `finally` block (or via
`CloseableUtils`) after the assertions. Also, `atLeast(4)` is brittle across
Curator/ZK versions and internal call paths; to assert retry occurred, it’s
usually enough to verify it was called more than once (e.g., `atLeast(2)`) or
to assert based on observable behavior rather than an exact-ish internal call
count.
##########
ql/src/test/org/apache/hadoop/hive/llap/TestProactiveEviction.java:
##########
@@ -0,0 +1,189 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.llap;
+
+import org.apache.commons.lang3.reflect.FieldUtils;
+import org.apache.curator.framework.CuratorFramework;
+import org.apache.curator.framework.CuratorFrameworkFactory;
+import org.apache.curator.framework.recipes.nodes.PersistentEphemeralNode;
+import org.apache.curator.retry.RetryOneTime;
+import org.apache.curator.test.TestingServer;
+import org.apache.curator.utils.CloseableUtils;
+import org.apache.hadoop.hive.conf.HiveConf;
+import org.apache.hadoop.hive.llap.io.api.LlapProxy;
+import org.apache.hadoop.hive.llap.registry.impl.LlapRegistryService;
+import org.apache.hadoop.hive.llap.registry.impl.LlapZookeeperRegistryImpl;
+import org.apache.hadoop.hive.registry.impl.ZkRegistryBase;
+import org.apache.hadoop.registry.client.binding.RegistryTypeUtils;
+import org.apache.hadoop.registry.client.binding.RegistryUtils;
+import org.apache.hadoop.registry.client.types.ServiceRecord;
+import org.apache.hadoop.security.UserGroupInformation;
+import org.junit.After;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.MockedStatic;
+import org.mockito.Mockito;
+
+import java.io.IOException;
+import java.net.InetSocketAddress;
+import java.net.URI;
+import java.util.Map;
+import java.util.concurrent.TimeUnit;
+
+
+import static org.junit.Assert.fail;
+
+/**
+ * Tests for {@link ProactiveEviction} focusing on the ZooKeeper-based LLAP
registry interaction
+ * with Kerberos authentication enabled.
+ *
+ * The tests use a local TestingServer (embedded ZooKeeper) and mock UGI to
simulate a secure
+ * environment without requiring a real KDC. The "llap-sasl" namespace is used
because
+ * HIVE_ZOOKEEPER_USE_KERBEROS is enabled, which is the namespace the registry
uses in production
+ * when Kerberos is active.
+ */
+public class TestProactiveEviction {
+
+ private HiveConf hiveConf = new HiveConf();
+
+ private CuratorFramework curatorFramework;
+ private TestingServer server;
+
+ private UserGroupInformation ugi;
+
+ MockedStatic<UserGroupInformation> userGroupInformationMockedStatic;
+
+ @Before
+ public void setUp() throws Exception {
+ ugi = Mockito.mock(UserGroupInformation.class);
+ userGroupInformationMockedStatic =
Mockito.mockStatic(UserGroupInformation.class);
+
userGroupInformationMockedStatic.when(UserGroupInformation::isSecurityEnabled).thenReturn(true);
+
userGroupInformationMockedStatic.when(UserGroupInformation::getCurrentUser).thenReturn(ugi);
+ Mockito.when(ugi.getShortUserName()).thenReturn("hive");
+
+ server = new TestingServer();
+ server.start();
+
+ hiveConf.setVar(HiveConf.ConfVars.LLAP_DAEMON_SERVICE_HOSTS,
"@testinstance");
+ hiveConf.setBoolVar(HiveConf.ConfVars.HIVE_ZOOKEEPER_USE_KERBEROS, true);
+ hiveConf.setVar(HiveConf.ConfVars.HIVE_ZOOKEEPER_QUORUM,
server.getConnectString());
+ hiveConf.setVar(HiveConf.ConfVars.HIVE_SERVER2_ZOOKEEPER_NAMESPACE,
"testinstance");
+ hiveConf.setVar(HiveConf.ConfVars.HIVE_ZOOKEEPER_NAMESPACE,
"testinstance");
+ hiveConf.setVar(HiveConf.ConfVars.LLAP_ZK_REGISTRY_USER, "hive");
+ hiveConf.setVar(HiveConf.ConfVars.HIVE_ZOOKEEPER_SESSION_TIMEOUT,
"1000ms");
+ hiveConf.setVar(HiveConf.ConfVars.LLAP_KERBEROS_PRINCIPAL,
"hive/host@REALM");
+ hiveConf.setVar(HiveConf.ConfVars.LLAP_KERBEROS_KEYTAB_FILE, "/keytab");
+ }
+
+ @After
+ public void tearDown() throws IOException {
+ server.stop();
+ userGroupInformationMockedStatic.close();
+ }
Review Comment:
Test cleanup is not robust if setup partially fails or if additional
resources are opened in tests (`curatorFramework`, `PersistentEphemeralNode`s).
Consider: (1) closing the `TestingServer` via
`close()`/`CloseableUtils.closeQuietly(server)` to ensure temp dirs and sockets
are released, (2) guarding cleanup with null checks, and (3) ensuring
`curatorFramework` (and any created nodes) are always closed in `finally`
blocks to avoid resource leaks and test flakiness.
##########
llap-client/src/test/org/apache/hadoop/hive/llap/registry/impl/TestLlapZookeeperRegistryImpl.java:
##########
@@ -99,6 +108,66 @@ public void testRegister() throws Exception {
parseInt(attributes.get(LlapRegistryService.LLAP_DAEMON_NUM_ENABLED_EXECUTORS)));
}
+ @Test
+ public void testRetryOnInvalidACLException() throws Exception {
+ // Given
+ LlapZookeeperRegistryImpl underTest =
+ new LlapZookeeperRegistryImpl("ClientRegistryRetryTest", hiveConf);
+
+ ACLProvider aclProvider = Mockito.mock(ACLProvider.class);
+ ACL allowAll = new ACL(ZooDefs.Perms.ALL, ZooDefs.Ids.ANYONE_ID_UNSAFE);
+ Mockito.when(aclProvider.getAclForPath(Mockito.any())).
+ thenReturn(Collections.emptyList()). // causes InvalidACLException
+ thenReturn(Collections.singletonList(allowAll)); // allow all
+
+ CuratorFramework curatorFrameworkWithAclProvider = CuratorFrameworkFactory.
+ builder().
+ connectString(server.getConnectString()).
+ sessionTimeoutMs(10000).
+ retryPolicy(new RetryOneTime(1000)).
+ aclProvider(aclProvider).
+ build();
+
+ trySetMock(underTest, "zooKeeperClient", curatorFrameworkWithAclProvider);
+ underTest.start();
+
+ // When
+ ServiceInstanceSet<LlapServiceInstance> serviceInstanceSet =
+ underTest.getInstances("LLAP", 10000);
+
+ // Then
+ Collection<LlapServiceInstance> llaps = serviceInstanceSet.getAll();
+ assertEquals(0, llaps.size());
+ Mockito.verify(aclProvider,
Mockito.atLeast(4)).getAclForPath(Mockito.any());
+ }
+
+ @Test
+ public void testClusterNotReadyExceptionIsThrownWhenZkNodeNotExists() throws
Exception {
Review Comment:
The test name/comment says the exception is thrown when the ZK node doesn’t
exist / timeout is exhausted, but the test passes a timeout of `0`, which
forces an immediate failure path rather than exercising ‘timeout exhausted’
behavior over time. Consider either renaming the test to reflect the ‘immediate
timeout’ semantics, or using a small positive timeout and asserting it retries
until the deadline (while keeping the test deterministic).
##########
llap-client/src/test/org/apache/hadoop/hive/llap/registry/impl/TestLlapZookeeperRegistryImpl.java:
##########
@@ -99,6 +108,66 @@ public void testRegister() throws Exception {
parseInt(attributes.get(LlapRegistryService.LLAP_DAEMON_NUM_ENABLED_EXECUTORS)));
}
+ @Test
+ public void testRetryOnInvalidACLException() throws Exception {
+ // Given
+ LlapZookeeperRegistryImpl underTest =
+ new LlapZookeeperRegistryImpl("ClientRegistryRetryTest", hiveConf);
+
+ ACLProvider aclProvider = Mockito.mock(ACLProvider.class);
+ ACL allowAll = new ACL(ZooDefs.Perms.ALL, ZooDefs.Ids.ANYONE_ID_UNSAFE);
+ Mockito.when(aclProvider.getAclForPath(Mockito.any())).
+ thenReturn(Collections.emptyList()). // causes InvalidACLException
+ thenReturn(Collections.singletonList(allowAll)); // allow all
+
+ CuratorFramework curatorFrameworkWithAclProvider = CuratorFrameworkFactory.
+ builder().
+ connectString(server.getConnectString()).
+ sessionTimeoutMs(10000).
+ retryPolicy(new RetryOneTime(1000)).
+ aclProvider(aclProvider).
+ build();
+
+ trySetMock(underTest, "zooKeeperClient", curatorFrameworkWithAclProvider);
+ underTest.start();
+
+ // When
+ ServiceInstanceSet<LlapServiceInstance> serviceInstanceSet =
+ underTest.getInstances("LLAP", 10000);
+
+ // Then
+ Collection<LlapServiceInstance> llaps = serviceInstanceSet.getAll();
+ assertEquals(0, llaps.size());
+ Mockito.verify(aclProvider,
Mockito.atLeast(4)).getAclForPath(Mockito.any());
+ }
+
+ @Test
+ public void testClusterNotReadyExceptionIsThrownWhenZkNodeNotExists() throws
Exception {
+ // Given
+ LlapZookeeperRegistryImpl underTest =
+ new LlapZookeeperRegistryImpl("ClientRegistryClusterNotReadyTest",
hiveConf);
+
+ ACLProvider aclProvider = Mockito.mock(ACLProvider.class);
+ List<ACL> secureAcls = new ArrayList<>();
+ secureAcls.addAll(ZooDefs.Ids.READ_ACL_UNSAFE); // Read all to the world
+ secureAcls.addAll(ZooDefs.Ids.CREATOR_ALL_ACL); //
Create/Delete/Write/Admin to creator
+
Mockito.when(aclProvider.getAclForPath(Mockito.any())).thenReturn(secureAcls);
+ CuratorFramework curatorFrameworkWithAclProvider = CuratorFrameworkFactory.
+ builder().
+ connectString(server.getConnectString()).
+ sessionTimeoutMs(10000).
+ retryPolicy(new RetryOneTime(1000)).
+ aclProvider(aclProvider).
+ build();
+
+ trySetMock(underTest, "zooKeeperClient", curatorFrameworkWithAclProvider);
+ underTest.start();
+
+ // When - Then
+ assertThrows(ClusterNotReadyException.class,
+ () -> underTest.getInstances("LLAP", 0));
Review Comment:
The test name/comment says the exception is thrown when the ZK node doesn’t
exist / timeout is exhausted, but the test passes a timeout of `0`, which
forces an immediate failure path rather than exercising ‘timeout exhausted’
behavior over time. Consider either renaming the test to reflect the ‘immediate
timeout’ semantics, or using a small positive timeout and asserting it retries
until the deadline (while keeping the test deterministic).
##########
llap-client/src/java/org/apache/hadoop/hive/registry/ClusterNotReadyException.java:
##########
@@ -0,0 +1,28 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.apache.hadoop.hive.registry;
+
+import java.io.IOException;
+
+public class ClusterNotReadyException extends IOException {
+
+ public ClusterNotReadyException(Throwable cause) {
+ super(cause);
+ }
Review Comment:
Consider adding `private static final long serialVersionUID = 1L;` to avoid
serialization warnings (common for exception types). Optionally add
constructors that accept a message (and message+cause) to make upstream
logs/errors more descriptive without relying solely on nested exception text.
--
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]
