JoegenUSTC commented on PR #11355: URL: https://github.com/apache/gravitino/pull/11355#issuecomment-4619534599
@diqiu50 Thanks for the thorough review! I've addressed all the feedback in the latest commit (`4502c3a`). Here's a summary of what was changed: **Simplified `createHiveClientImpl()` (re: "HiveVersion is a shared class")** Removed the unnecessary `Enum.valueOf` reflection workaround. You are correct — `HiveClientClassLoader.isSharedClass()` has an explicit rule covering all `org.apache.gravitino.*` classes, so `HiveVersion` is always delegated to the base classloader. Both sides hold the same `Class` object and we can pass `HiveVersion.class` directly to `getConstructor()`. **Removed `TestHiveClientFactoryClassLoader` (re: "Tests for TCCL are not necessary" and "HiveVersion is a shared class. This is not a problem")** Deleted the entire test file. The two TCCL tests were verifying Java language guarantees rather than our code's behaviour, and the two Bug 2 tests were built on the incorrect assumption that `HiveVersion` is not a shared class. **Replaced FQN with a proper import in `HiveClientFactory.java` (re: "FQN")** Added `import java.security.PrivilegedExceptionAction;` at the top of the file per the project coding standards. **Strengthened `KerberosClient.getRealLoginUgi()` contract (Copilot suggestion)** Added `Preconditions.checkState(realLoginUgi != null, ...)` inside the method so callers fail fast with a clear error message if `login()` was not called. Updated the Javadoc to document the `@throws IllegalStateException` contract. All changes compile cleanly and the unit tests (`hive-metastore-common:test`) and spotless format check pass locally. Please take another look when you have a chance! -- 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]
