JoegenUSTC commented on code in PR #11355: URL: https://github.com/apache/gravitino/pull/11355#discussion_r3353880962
########## catalogs/hive-metastore-common/src/test/java/org/apache/gravitino/hive/client/TestHiveClientFactoryClassLoader.java: ########## @@ -0,0 +1,208 @@ +/* + * 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.gravitino.hive.client; + +import static org.junit.jupiter.api.Assertions.assertNotSame; +import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; +import static org.junit.jupiter.api.Assertions.assertTrue; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +import java.util.Properties; +import org.junit.jupiter.api.Test; + +/** + * Unit tests for ClassLoader-related fixes in {@link HiveClientFactory}. + * + * <p>These tests verify the three bug fixes without requiring a live HMS or KDC: + * + * <ul> + * <li>Bug 1: {@code createHiveClientWithBackend()} uses a stable {@code baseLoader} ( {@code + * HiveClientFactory.class.getClassLoader()}) instead of the unstable TCCL. + * <li>Bug 2: {@code createHiveClientImpl()} loads {@code HiveVersion} from the isolated + * ClassLoader to avoid {@link NoSuchMethodException}. + * </ul> + * + * <p>Bug 3 (missing {@code doAs} in non-impersonation Kerberos path) requires a live KDC and is + * covered by the integration test {@link TestHive2HMSWithKerberosNoImpersonation}. + */ +public class TestHiveClientFactoryClassLoader { + + // --------------------------------------------------------------------------- + // Bug 1: factory CL is stable; TCCL is not — contrast/rationale tests + // --------------------------------------------------------------------------- + + /** + * Verifies that {@code HiveClientFactory.class.getClassLoader()} is a stable, consistent + * reference across threads. + * + * <p>This is the positive side of the Bug 1 fix: it documents why the factory CL is the correct + * choice as {@code baseLoader} for {@link HiveClientClassLoader}. + */ + @Test + public void testFactoryClassLoaderIsStableAcrossThreads() throws InterruptedException { + ClassLoader expectedCl = HiveClientFactory.class.getClassLoader(); + ClassLoader[] captured = new ClassLoader[1]; + + Thread t = new Thread(() -> captured[0] = HiveClientFactory.class.getClassLoader()); + // Even if the thread has a different TCCL, the factory CL must remain the same + t.setContextClassLoader(new ClassLoader(expectedCl) {}); + t.start(); + t.join(); + + assertSame( + expectedCl, + captured[0], + "HiveClientFactory.class.getClassLoader() must be identical across threads"); + } + + /** + * Demonstrates that TCCL is <em>not</em> stable across threads — which is exactly why Bug 1 + * existed. + * + * <p>If two threads have different TCCLs and TCCL were used as {@code baseLoader}, {@code + * UserGroupInformation} (a shared hadoop class delegated via {@code isSharedClass}) would be + * resolved against different ClassLoaders, making their static TGT state invisible to each other. + */ + @Test + public void testTCCLIsUnstableAcrossThreads() throws InterruptedException { + ClassLoader factoryCl = HiveClientFactory.class.getClassLoader(); + ClassLoader customTccl = new ClassLoader(factoryCl) {}; + ClassLoader[] capturedTccl = new ClassLoader[1]; + + Thread t = new Thread(() -> capturedTccl[0] = Thread.currentThread().getContextClassLoader()); + t.setContextClassLoader(customTccl); + t.start(); + t.join(); + + // The thread's TCCL is the custom one we set — different from the factory CL + assertSame(customTccl, capturedTccl[0], "Thread TCCL should reflect what was set"); + assertNotSame( + factoryCl, + capturedTccl[0], + "TCCL can differ from factory CL, proving it is not a stable baseLoader reference"); + } + + // --------------------------------------------------------------------------- + // Bug 2: createHiveClientImpl() must use isolated CL's HiveVersion enum + // --------------------------------------------------------------------------- + + /** + * Verifies that {@link HiveClientFactory#createHiveClientImpl} loads {@code HiveVersion} from the + * provided {@code classloader} before calling {@link Class#getConstructor}. + * + * <p>The mock classloader returns the real {@code HiveVersion} class for the {@code + * HiveVersion.loadClass} call, and throws a sentinel {@link ClassNotFoundException} for {@code + * HiveClientImpl.loadClass}. The test asserts that the sentinel is thrown (not {@link + * NoSuchMethodException}), proving that the {@code HiveVersion} lookup on the isolated CL + * succeeded before reaching the constructor step. + * + * <p>If the bug were present, {@code getConstructor} would receive the system CL's {@code + * HiveVersion} and throw {@link NoSuchMethodException} instead. + */ + @Test + @SuppressWarnings({"unchecked", "rawtypes"}) + public void testCreateHiveClientImplLoadsHiveVersionFromIsolatedClassLoader() throws Exception { + ClassLoader mockCl = mock(ClassLoader.class); Review Comment: Correct, the Bug 2 fix was based on a wrong assumption. Since `HiveVersion` is a shared class (covered by the `org.apache.gravitino.*` rule in `isSharedClass()`), the isolated classloader never redefines it — both the factory and the isolated classloader see the same `Class` object. The corresponding tests (`testCreateHiveClientImplLoadsHiveVersionFromIsolatedClassLoader` and `testSystemClHiveVersionCausesNoSuchMethodException`) have been removed along with the production code workaround. -- 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]
