diqiu50 commented on code in PR #11355:
URL: https://github.com/apache/gravitino/pull/11355#discussion_r3353255833
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveClientFactory.java:
##########
@@ -162,10 +164,18 @@ public static HiveClient createHiveClientImpl(
HiveClientClassLoader.HiveVersion version, Properties properties,
ClassLoader classloader)
throws Exception {
Class<?> hiveClientImplClass =
classloader.loadClass(HiveClientImpl.class.getName());
+ // HiveClientImpl is a barrier class loaded via defineClass inside the
isolated classloader.
+ // Its constructor expects the HiveVersion enum type from the *isolated*
classloader scope.
+ // Passing the system classloader's HiveVersion.class causes
NoSuchMethodException because
+ // the two Class objects are not identical even though they have the same
name.
+ Class<?> hiveVersionInIsolated =
+
classloader.loadClass(HiveClientClassLoader.HiveVersion.class.getName());
+ @SuppressWarnings({"unchecked", "rawtypes"})
+ Object isolatedVersion =
+ Enum.valueOf((Class<? extends Enum>) hiveVersionInIsolated,
version.name());
Constructor<?> hiveClientImplCtor =
- hiveClientImplClass.getConstructor(
- HiveClientClassLoader.HiveVersion.class, Properties.class);
- return (HiveClient) hiveClientImplCtor.newInstance(version, properties);
Review Comment:
`HiveClientClassLoader.HiveVersion.class` is shared class. We can pass to
the `classloader` directly
##########
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
+ //
---------------------------------------------------------------------------
Review Comment:
Tests for TCCL are not necessary.
##########
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:
HiveVersion is a shared class. This is not a problem
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveClientFactory.java:
##########
@@ -202,6 +212,17 @@ private HiveClient
createHiveClientInternal(HiveClientClassLoader classloader) {
return createProxyHiveClientImpl(
classloader.getHiveVersion(), properties, ugi, classloader);
+ } else if (enableKerberos) {
+ // UGI is a shared class (org.apache.hadoop.* delegated to
baseLoader), so the system CL
+ // and HiveClientClassLoader share the same UGI static state. The TGT
is already stored in
+ // realLoginUgi.subject by kerberosClient.login(). The only thing
needed is to bind that
+ // Subject to the current thread so GSSAPI can find the TGT during the
HMS Thrift handshake.
+ // UGI.doAs() wraps Subject.doAs() internally — same pattern as
ImpalaEngineAdapter.
+ UserGroupInformation realUgi = kerberosClient.getRealLoginUgi();
+ final HiveClientClassLoader.HiveVersion hiveVersion =
classloader.getHiveVersion();
+ return realUgi.doAs(
+ (java.security.PrivilegedExceptionAction<HiveClient>)
Review Comment:
FQN
--
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]
