Copilot commented on code in PR #9499:
URL: https://github.com/apache/gravitino/pull/9499#discussion_r2642477777
##########
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive3IT.java:
##########
@@ -21,9 +21,15 @@
import com.google.common.collect.ImmutableMap;
import org.apache.gravitino.integration.test.container.HiveContainer;
import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.TestInstance;
@Tag("gravitino-docker-test")
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
public class CatalogHive3IT extends CatalogHive2IT {
+ {
Review Comment:
Using an instance initializer block to set hmsCatalog is unconventional and
can make the code harder to understand. Consider initializing hmsCatalog
directly as a field declaration or in a constructor/setup method for better
clarity and maintainability.
```suggestion
public CatalogHive3IT() {
```
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveShimV3.java:
##########
@@ -441,6 +466,23 @@ private Object invoke(ExceptionTarget target, Object
object, Method method, Obje
}
}
+ /**
+ * Creates an object using a constructor and applies a customizer, then
converts any exception to
+ * a Gravitino exception.
+ *
+ * @param target Hive object info used in error messages and exception
mapping
+ * @param constructor The constructor to use for creating the object
+ * @param args The arguments to pass to the constructor
+ * @return The created object
+ */
Review Comment:
The javadoc says "Creates an object using a constructor and applies a
customizer" but there is no customizer being applied. The description should be
updated to match the actual implementation which just creates an object using
the constructor.
##########
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHiveS3IT.java:
##########
@@ -29,12 +29,14 @@
import org.apache.hadoop.conf.Configuration;
import org.apache.hadoop.fs.FileSystem;
import org.apache.spark.sql.SparkSession;
+import org.junit.jupiter.api.TestInstance;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.testcontainers.containers.Container;
import org.testcontainers.shaded.org.awaitility.Awaitility;
-public class CatalogHiveS3IT extends CatalogHive2IT {
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+public class CatalogHiveS3IT extends CatalogHive3IT {
Review Comment:
CatalogHiveS3IT now extends CatalogHive3IT (changed from CatalogHive2IT).
This is a significant behavioral change that should be verified. Ensure that
all S3-related functionality works correctly with Hive3, especially considering
differences in catalog support and metastore behavior between Hive2 and Hive3.
##########
catalogs/catalog-hive/src/test/java/org/apache/gravitino/catalog/hive/integration/test/CatalogHive3ITWithCatalog.java:
##########
@@ -0,0 +1,73 @@
+/*
+ * 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.catalog.hive.integration.test;
+
+import static org.apache.gravitino.catalog.hive.HiveConstants.DEFAULT_CATALOG;
+import static org.apache.gravitino.catalog.hive.HiveConstants.METASTORE_URIS;
+
+import com.google.common.collect.Maps;
+import java.util.Map;
+import org.apache.gravitino.Catalog;
+import org.apache.gravitino.integration.test.container.HiveContainer;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.TestInstance;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+@Tag("gravitino-docker-test")
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+public class CatalogHive3ITWithCatalog extends CatalogHive3IT {
+
+ private static final Logger LOGGER =
LoggerFactory.getLogger(CatalogHive3ITWithCatalog.class);
+
+ @Override
+ protected void startNecessaryContainer() {
+ super.startNecessaryContainer();
+ hmsCatalog = "mycatalog";
+ enableSparkTest = false;
+ }
+
+ @Override
+ protected void createCatalog() {
+
+ String location =
+ String.format(
+ "hdfs://%s:%d/user/hive/warehouse/%s" + hmsCatalog,
+ containerSuite.getHiveContainer().getContainerIpAddress(),
+ HiveContainer.HDFS_DEFAULTFS_PORT,
+ schemaName.toLowerCase());
Review Comment:
String concatenation issue in the location path. The format string has '%s'
+ hmsCatalog which will concatenate the variable name instead of inserting it
into the format string. This should either use an additional format specifier
or be concatenated properly.
```suggestion
"hdfs://%s:%d/user/hive/warehouse/%s",
containerSuite.getHiveContainer().getContainerIpAddress(),
HiveContainer.HDFS_DEFAULTFS_PORT,
schemaName.toLowerCase())
+ hmsCatalog;
```
##########
catalogs/hive-metastore-common/src/test/java/org/apache/gravitino/hive/client/TestHive2HMSWithKerberos.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * 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 java.io.File;
+import java.lang.reflect.Method;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.util.Properties;
+import org.apache.commons.io.FileUtils;
+import org.apache.gravitino.integration.test.container.HiveContainer;
+import org.apache.hadoop.security.authentication.util.KerberosName;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.TestInstance;
+
+/** Kerberos-enabled Hive2 HMS tests. */
+@Tag("gravitino-docker-test")
+@TestInstance(TestInstance.Lifecycle.PER_CLASS)
+public class TestHive2HMSWithKerberos extends TestHive2HMS {
+
+ private static final String CLIENT_PRINCIPAL = "cli@HADOOPKRB";
+ private File tempDir;
+ private String keytabPath;
+ private String krb5ConfPath;
+
+ @BeforeAll
+ @Override
+ public void startHiveContainer() {
+ testPrefix = "hive2_kerberos";
+ catalogName = "";
+ containerSuite.startKerberosHiveContainer();
+ hiveContainer = containerSuite.getKerberosHiveContainer();
+
+ metastoreUri =
+ String.format(
+ "thrift://%s:%d",
+ hiveContainer.getContainerIpAddress(),
HiveContainer.HIVE_METASTORE_PORT);
+ hdfsBasePath =
+ String.format(
+ "hdfs://%s:%d/tmp/gravitino_test",
+ hiveContainer.getContainerIpAddress(),
HiveContainer.HDFS_DEFAULTFS_PORT);
+
+ prepareKerberosConfig();
+
+ // Initialize client with Kerberos-aware properties.
+ hiveClient = new HiveClientFactory(createHiveProperties(),
"").createHiveClient();
+ }
+
+ protected void prepareKerberosConfig() {
+ try {
+ tempDir = Files.createTempDirectory(testPrefix).toFile();
+ tempDir.deleteOnExit();
Review Comment:
Potential resource leak: tempDir is created in prepareKerberosConfig() and
marked with deleteOnExit(), but this may not properly clean up temporary files
if the JVM terminates abnormally. Consider adding cleanup logic in an @AfterAll
method to explicitly delete the temporary directory and its contents.
##########
catalogs/hive-metastore-common/src/main/java/org/apache/gravitino/hive/client/HiveClientFactory.java:
##########
@@ -80,8 +80,8 @@ public HiveClientFactory(Properties properties, String name) {
kerberosClient.setHiveClient(client);
}
} catch (Exception e) {
- throw new RuntimeException(
- String.format("Failed to initialize HiveClientFactory %s",
this.name), e);
+ throw HiveExceptionConverter.toGravitinoException(
+ e, HiveExceptionConverter.ExceptionTarget.other(this.name));
}
Review Comment:
The exception handling in HiveClientFactory initialization should use
HiveExceptionConverter for consistency. However, verify that converting to
GravitinoException at this early stage won't cause issues with downstream error
handling, especially for connection-related failures during factory
initialization.
--
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]
