g3rg0 commented on code in PR #6677: URL: https://github.com/apache/hive/pull/6677#discussion_r3728799944
########## 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: Done. The redundant server.start() call has been removed in the second commit. We now rely solely on the TestingServer() default constructor which auto-starts the server. This avoids the risk of inconsistent behavior or exceptions from double-starting across different Curator versions. ########## 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: Done. Cleanup has been made robust across the board: 1. tearDown() now uses CloseableUtils.closeQuietly() for both curatorFramework and TestingServer, ensuring temp dirs and sockets are released even if an exception occurs during cleanup itself. 2. All resource fields are null-guarded before closing, so partial setup failures won't cause NPEs in teardown. 3. Test methods that create their own CuratorFramework instances (e.g. testRetryOnInvalidACLException, the ClusterNotReadyException tests) now wrap usage in try/finally blocks with CloseableUtils.closeQuietly(), preventing thread and socket leaks regardless of test outcome. 4. PersistentEphemeralNode instances are closed via CloseableUtils.closeQuietly() at the end of the test that creates them. ########## 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: Done. Both concerns addressed: 1. curatorFrameworkWithAclProvider is now closed in a finally block via CloseableUtils.closeQuietly(), preventing thread/socket leaks regardless of whether the test passes or throws. 2. The verification threshold has been relaxed from atLeast(4) to atLeast(2). This still asserts that a retry occurred (i.e., the method was called more than once) without being brittle to internal call-count differences across Curator/ZK versions. ########## 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: Done. See the previous comment. -- 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]
