Copilot commented on code in PR #9900: URL: https://github.com/apache/gravitino/pull/9900#discussion_r2771668650
########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ClosableJdbcCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.iceberg.common; + +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.security.PrivilegedExceptionAction; +import java.util.Map; +import java.util.function.Function; +import org.apache.gravitino.iceberg.common.authentication.AuthenticationConfig; +import org.apache.gravitino.iceberg.common.authentication.SupportsKerberos; +import org.apache.gravitino.iceberg.common.authentication.kerberos.KerberosClient; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.jdbc.JdbcClientPool; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * ClosableHiveCatalog is a wrapper class to wrap Iceberg HiveCatalog to do some clean-up work like + * closing resources. + */ Review Comment: The JavaDoc comment still refers to "ClosableHiveCatalog" but this class is "ClosableJdbcCatalog". The documentation should be updated to reflect that this is a wrapper for Iceberg JdbcCatalog, not HiveCatalog. ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/utils/IcebergCatalogUtil.java: ########## @@ -101,15 +102,28 @@ private static JdbcCatalog loadJdbcCatalog(IcebergConfig icebergConfig) { } catch (ClassNotFoundException e) { throw new IllegalArgumentException("Couldn't load jdbc driver " + driverClassName); } - JdbcCatalog jdbcCatalog = + ClosableJdbcCatalog jdbcCatalog = new JdbcCatalogWithMetadataLocationSupport( icebergConfig.get(IcebergConfig.JDBC_INIT_TABLES)); HdfsConfiguration hdfsConfiguration = new HdfsConfiguration(); properties.forEach(hdfsConfiguration::set); - jdbcCatalog.setConf(hdfsConfiguration); + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); try { - jdbcCatalog.initialize(icebergCatalogName, properties); + if (authenticationConfig.isSimpleAuth()) { + jdbcCatalog.setConf(hdfsConfiguration); + jdbcCatalog.setHadoopConf(hdfsConfiguration); + jdbcCatalog.initialize(icebergCatalogName, properties); + } else if (authenticationConfig.isKerberosAuth()) { + hdfsConfiguration.set(HADOOP_SECURITY_AUTHORIZATION, "true"); + hdfsConfiguration.set(HADOOP_SECURITY_AUTHENTICATION, "kerberos"); Review Comment: For Kerberos authentication in HiveCatalog (lines 80-81 in IcebergCatalogUtil), the property CLIENT_POOL_CACHE_KEYS is set to "USER_NAME". However, this property is not set for JdbcCatalog when using Kerberos authentication. This inconsistency may cause issues with connection pool caching in multi-user Kerberos environments. Consider adding the same property for JDBC Kerberos authentication to maintain consistency. ```suggestion hdfsConfiguration.set(HADOOP_SECURITY_AUTHENTICATION, "kerberos"); // Ensure client pool caching is keyed by user name in Kerberos environments, // consistent with the Hive catalog configuration. properties.put("client.pool.cache-keys", "USER_NAME"); ``` ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ClosableJdbcCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.iceberg.common; + +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.security.PrivilegedExceptionAction; +import java.util.Map; +import java.util.function.Function; +import org.apache.gravitino.iceberg.common.authentication.AuthenticationConfig; +import org.apache.gravitino.iceberg.common.authentication.SupportsKerberos; +import org.apache.gravitino.iceberg.common.authentication.kerberos.KerberosClient; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.jdbc.JdbcClientPool; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * ClosableHiveCatalog is a wrapper class to wrap Iceberg HiveCatalog to do some clean-up work like + * closing resources. + */ +public class ClosableJdbcCatalog extends JdbcCatalog implements Closeable, SupportsKerberos { + + private static final Logger LOGGER= LoggerFactory.getLogger(ClosableJdbcCatalog.class); + + private KerberosClient kerberosClient; + + private Configuration hadoopConf; + + public ClosableJdbcCatalog() { + super(); + } + + public ClosableJdbcCatalog( + Function<Map<String, String>, FileIO> ioBuilder, + Function<Map<String, String>, JdbcClientPool> clientPoolBuilder, + boolean initializeCatalogTables) { + super(ioBuilder, clientPoolBuilder, initializeCatalogTables); + } + + /** + * Initialize the ClosableHiveCatalog with the given input name and properties. + * + * <p>Note: This method can only be called once as it will create new client pools. + * + * @param inputName name of the catalog + * @param properties properties for the catalog + */ + @Override + public void initialize(String inputName, Map<String, String> properties) { + super.initialize(inputName, properties); + + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); + if (authenticationConfig.isKerberosAuth()) { + this.kerberosClient = initKerberosClient(); + } + } + + public Configuration getHadoopConf() { + return hadoopConf; + } + + public void setHadoopConf(Configuration hadoopConf) { + this.hadoopConf = hadoopConf; + } + + @Override + public void close() { + if (kerberosClient != null) { + try { + kerberosClient.close(); + } catch (Exception e) { + LOGGER.warn("Failed to close KerberosClient", e); + } + } + } Review Comment: Unlike ClosableHiveCatalog which has a resources list and addResource method for managing additional closeable resources (see ClosableHiveCatalog lines 58, 66-68), ClosableJdbcCatalog does not provide this functionality. This means there's no way to register and properly close additional resources that may be needed by JDBC catalog users. Consider adding the same resource management capability for consistency and extensibility. ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ClosableJdbcCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.iceberg.common; + +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.security.PrivilegedExceptionAction; +import java.util.Map; +import java.util.function.Function; +import org.apache.gravitino.iceberg.common.authentication.AuthenticationConfig; +import org.apache.gravitino.iceberg.common.authentication.SupportsKerberos; +import org.apache.gravitino.iceberg.common.authentication.kerberos.KerberosClient; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.jdbc.JdbcClientPool; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * ClosableHiveCatalog is a wrapper class to wrap Iceberg HiveCatalog to do some clean-up work like + * closing resources. + */ +public class ClosableJdbcCatalog extends JdbcCatalog implements Closeable, SupportsKerberos { + + private static final Logger LOGGER= LoggerFactory.getLogger(ClosableJdbcCatalog.class); + + private KerberosClient kerberosClient; + + private Configuration hadoopConf; + + public ClosableJdbcCatalog() { + super(); + } + + public ClosableJdbcCatalog( + Function<Map<String, String>, FileIO> ioBuilder, + Function<Map<String, String>, JdbcClientPool> clientPoolBuilder, + boolean initializeCatalogTables) { + super(ioBuilder, clientPoolBuilder, initializeCatalogTables); + } + + /** + * Initialize the ClosableHiveCatalog with the given input name and properties. + * + * <p>Note: This method can only be called once as it will create new client pools. + * + * @param inputName name of the catalog + * @param properties properties for the catalog + */ + @Override + public void initialize(String inputName, Map<String, String> properties) { + super.initialize(inputName, properties); + + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); + if (authenticationConfig.isKerberosAuth()) { + this.kerberosClient = initKerberosClient(); + } + } + + public Configuration getHadoopConf() { + return hadoopConf; + } + + public void setHadoopConf(Configuration hadoopConf) { + this.hadoopConf = hadoopConf; + } + + @Override + public void close() { + if (kerberosClient != null) { + try { + kerberosClient.close(); + } catch (Exception e) { + LOGGER.warn("Failed to close KerberosClient", e); + } + } + } + + @Override + public <R> R doKerberosOperations(Executable<R> executable) throws Throwable { + Map<String, String> properties = this.properties(); + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); + + final String finalPrincipalName; + String proxyKerberosPrincipalName = PrincipalUtils.getCurrentPrincipal().getName(); + + if (!proxyKerberosPrincipalName.contains("@")) { + finalPrincipalName = + String.format("%s@%s", proxyKerberosPrincipalName, kerberosClient.getRealm()); + } else { + finalPrincipalName = proxyKerberosPrincipalName; + } + + UserGroupInformation realUser = + authenticationConfig.isImpersonationEnabled() + ? UserGroupInformation.createProxyUser( + finalPrincipalName, kerberosClient.getLoginUser()) + : kerberosClient.getLoginUser(); + + return realUser.doAs( + (PrivilegedExceptionAction<R>) + () -> { + try { + return executable.execute(); + } catch (Throwable e) { + if (RuntimeException.class.isAssignableFrom(e.getClass())) { + throw (RuntimeException) e; + } + throw new RuntimeException("Failed to invoke method", e); + } + }); + } + + private KerberosClient initKerberosClient() { + try { + KerberosClient kerberosClient = new KerberosClient(this.properties(), this.getHadoopConf()); + // catalog_uuid always exists for Gravitino managed catalogs, `0` is just a fallback value. + String catalogUUID = properties().getOrDefault("catalog_uuid", "0"); + File keytabFile = kerberosClient.saveKeyTabFileFromUri(Long.parseLong(catalogUUID)); + kerberosClient.login(keytabFile.getAbsolutePath()); + return kerberosClient; + } catch (IOException e) { + throw new RuntimeException("Failed to login with kerberos", e); + } + } +} Review Comment: The new ClosableJdbcCatalog class and its Kerberos authentication functionality lack test coverage. While there is a test for basic JDBC catalog loading in TestIcebergCatalogUtil, there are no tests for: 1) ClosableJdbcCatalog initialization with Kerberos configuration, 2) doKerberosOperations method, 3) close() method cleanup, or 4) the integration of JDBC catalog with HDFS Kerberos authentication. Consider adding tests to verify the Kerberos authentication flow and resource cleanup work correctly. ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/utils/IcebergCatalogUtil.java: ########## @@ -101,15 +102,28 @@ private static JdbcCatalog loadJdbcCatalog(IcebergConfig icebergConfig) { } catch (ClassNotFoundException e) { throw new IllegalArgumentException("Couldn't load jdbc driver " + driverClassName); } - JdbcCatalog jdbcCatalog = + ClosableJdbcCatalog jdbcCatalog = new JdbcCatalogWithMetadataLocationSupport( icebergConfig.get(IcebergConfig.JDBC_INIT_TABLES)); HdfsConfiguration hdfsConfiguration = new HdfsConfiguration(); properties.forEach(hdfsConfiguration::set); - jdbcCatalog.setConf(hdfsConfiguration); + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); try { - jdbcCatalog.initialize(icebergCatalogName, properties); + if (authenticationConfig.isSimpleAuth()) { + jdbcCatalog.setConf(hdfsConfiguration); + jdbcCatalog.setHadoopConf(hdfsConfiguration); + jdbcCatalog.initialize(icebergCatalogName, properties); + } else if (authenticationConfig.isKerberosAuth()) { + hdfsConfiguration.set(HADOOP_SECURITY_AUTHORIZATION, "true"); + hdfsConfiguration.set(HADOOP_SECURITY_AUTHENTICATION, "kerberos"); + jdbcCatalog.setConf(hdfsConfiguration); + jdbcCatalog.setHadoopConf(hdfsConfiguration); + jdbcCatalog.initialize(icebergCatalogName, properties); + } else { + throw new UnsupportedOperationException( + "Unsupported authentication method: " + authenticationConfig.getAuthType()); + } Review Comment: The code duplicates the same three lines (setConf, setHadoopConf, initialize) in both the simple and kerberos authentication branches. Consider extracting this common logic after the authentication-specific configuration to reduce code duplication. For example, set the authentication-specific HADOOP_SECURITY_* properties first, then call the common setup code. ```suggestion // No additional configuration needed for simple authentication. } else if (authenticationConfig.isKerberosAuth()) { hdfsConfiguration.set(HADOOP_SECURITY_AUTHORIZATION, "true"); hdfsConfiguration.set(HADOOP_SECURITY_AUTHENTICATION, "kerberos"); } else { throw new UnsupportedOperationException( "Unsupported authentication method: " + authenticationConfig.getAuthType()); } jdbcCatalog.setConf(hdfsConfiguration); jdbcCatalog.setHadoopConf(hdfsConfiguration); jdbcCatalog.initialize(icebergCatalogName, properties); ``` ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ClosableJdbcCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.iceberg.common; + +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.security.PrivilegedExceptionAction; +import java.util.Map; +import java.util.function.Function; +import org.apache.gravitino.iceberg.common.authentication.AuthenticationConfig; +import org.apache.gravitino.iceberg.common.authentication.SupportsKerberos; +import org.apache.gravitino.iceberg.common.authentication.kerberos.KerberosClient; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.jdbc.JdbcClientPool; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * ClosableHiveCatalog is a wrapper class to wrap Iceberg HiveCatalog to do some clean-up work like + * closing resources. + */ +public class ClosableJdbcCatalog extends JdbcCatalog implements Closeable, SupportsKerberos { + + private static final Logger LOGGER= LoggerFactory.getLogger(ClosableJdbcCatalog.class); + + private KerberosClient kerberosClient; + + private Configuration hadoopConf; + + public ClosableJdbcCatalog() { + super(); + } + + public ClosableJdbcCatalog( + Function<Map<String, String>, FileIO> ioBuilder, + Function<Map<String, String>, JdbcClientPool> clientPoolBuilder, + boolean initializeCatalogTables) { + super(ioBuilder, clientPoolBuilder, initializeCatalogTables); + } + + /** + * Initialize the ClosableHiveCatalog with the given input name and properties. + * + * <p>Note: This method can only be called once as it will create new client pools. + * + * @param inputName name of the catalog + * @param properties properties for the catalog + */ + @Override + public void initialize(String inputName, Map<String, String> properties) { + super.initialize(inputName, properties); + + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); + if (authenticationConfig.isKerberosAuth()) { + this.kerberosClient = initKerberosClient(); + } + } + + public Configuration getHadoopConf() { + return hadoopConf; + } + + public void setHadoopConf(Configuration hadoopConf) { + this.hadoopConf = hadoopConf; + } + + @Override + public void close() { + if (kerberosClient != null) { + try { + kerberosClient.close(); + } catch (Exception e) { + LOGGER.warn("Failed to close KerberosClient", e); + } + } + } + + @Override + public <R> R doKerberosOperations(Executable<R> executable) throws Throwable { + Map<String, String> properties = this.properties(); + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); + + final String finalPrincipalName; + String proxyKerberosPrincipalName = PrincipalUtils.getCurrentPrincipal().getName(); + + if (!proxyKerberosPrincipalName.contains("@")) { + finalPrincipalName = + String.format("%s@%s", proxyKerberosPrincipalName, kerberosClient.getRealm()); + } else { + finalPrincipalName = proxyKerberosPrincipalName; + } + + UserGroupInformation realUser = + authenticationConfig.isImpersonationEnabled() + ? UserGroupInformation.createProxyUser( + finalPrincipalName, kerberosClient.getLoginUser()) + : kerberosClient.getLoginUser(); Review Comment: In the doKerberosOperations method, there's a potential NullPointerException if kerberosClient is null when calling kerberosClient.getRealm() or kerberosClient.getLoginUser(). This could happen if the method is called when authentication is not Kerberos. Consider adding a null check for kerberosClient at the start of the method or ensuring this method is only called after proper initialization. ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ClosableJdbcCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.iceberg.common; + +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.security.PrivilegedExceptionAction; +import java.util.Map; +import java.util.function.Function; +import org.apache.gravitino.iceberg.common.authentication.AuthenticationConfig; +import org.apache.gravitino.iceberg.common.authentication.SupportsKerberos; +import org.apache.gravitino.iceberg.common.authentication.kerberos.KerberosClient; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.jdbc.JdbcClientPool; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * ClosableHiveCatalog is a wrapper class to wrap Iceberg HiveCatalog to do some clean-up work like + * closing resources. + */ +public class ClosableJdbcCatalog extends JdbcCatalog implements Closeable, SupportsKerberos { + + private static final Logger LOGGER= LoggerFactory.getLogger(ClosableJdbcCatalog.class); + + private KerberosClient kerberosClient; + + private Configuration hadoopConf; + + public ClosableJdbcCatalog() { + super(); + } + + public ClosableJdbcCatalog( + Function<Map<String, String>, FileIO> ioBuilder, + Function<Map<String, String>, JdbcClientPool> clientPoolBuilder, + boolean initializeCatalogTables) { + super(ioBuilder, clientPoolBuilder, initializeCatalogTables); + } + + /** + * Initialize the ClosableHiveCatalog with the given input name and properties. + * + * <p>Note: This method can only be called once as it will create new client pools. + * + * @param inputName name of the catalog + * @param properties properties for the catalog + */ + @Override + public void initialize(String inputName, Map<String, String> properties) { + super.initialize(inputName, properties); + + AuthenticationConfig authenticationConfig = new AuthenticationConfig(properties); + if (authenticationConfig.isKerberosAuth()) { + this.kerberosClient = initKerberosClient(); + } + } + + public Configuration getHadoopConf() { + return hadoopConf; + } + + public void setHadoopConf(Configuration hadoopConf) { + this.hadoopConf = hadoopConf; + } + + @Override + public void close() { + if (kerberosClient != null) { + try { + kerberosClient.close(); + } catch (Exception e) { + LOGGER.warn("Failed to close KerberosClient", e); Review Comment: The close() method in ClosableJdbcCatalog only closes the KerberosClient, unlike ClosableHiveCatalog which also closes internal client pools and additional resources. JdbcCatalog likely has internal JDBC connection pools that should be properly closed to prevent resource leaks. Consider adding cleanup logic for JDBC connection pools similar to ClosableHiveCatalog's closeInternalClientPool() method. ```suggestion try { super.close(); } catch (RuntimeException e) { LOGGER.warn("Failed to close JdbcCatalog resources", e); } finally { if (kerberosClient != null) { try { kerberosClient.close(); } catch (Exception e) { LOGGER.warn("Failed to close KerberosClient", e); } ``` ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ClosableJdbcCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.iceberg.common; + +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.security.PrivilegedExceptionAction; +import java.util.Map; +import java.util.function.Function; +import org.apache.gravitino.iceberg.common.authentication.AuthenticationConfig; +import org.apache.gravitino.iceberg.common.authentication.SupportsKerberos; +import org.apache.gravitino.iceberg.common.authentication.kerberos.KerberosClient; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.jdbc.JdbcClientPool; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * ClosableHiveCatalog is a wrapper class to wrap Iceberg HiveCatalog to do some clean-up work like + * closing resources. + */ +public class ClosableJdbcCatalog extends JdbcCatalog implements Closeable, SupportsKerberos { + + private static final Logger LOGGER= LoggerFactory.getLogger(ClosableJdbcCatalog.class); Review Comment: Missing space after "=" in logger initialization. According to Google Java Style Guide, there should be a space on both sides of binary operators. This should be "LOGGER = " instead of "LOGGER= ". ```suggestion private static final Logger LOGGER = LoggerFactory.getLogger(ClosableJdbcCatalog.class); ``` ########## iceberg/iceberg-common/src/main/java/org/apache/gravitino/iceberg/common/ClosableJdbcCatalog.java: ########## @@ -0,0 +1,147 @@ +/* + * 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.iceberg.common; + +import java.io.Closeable; +import java.io.File; +import java.io.IOException; +import java.security.PrivilegedExceptionAction; +import java.util.Map; +import java.util.function.Function; +import org.apache.gravitino.iceberg.common.authentication.AuthenticationConfig; +import org.apache.gravitino.iceberg.common.authentication.SupportsKerberos; +import org.apache.gravitino.iceberg.common.authentication.kerberos.KerberosClient; +import org.apache.gravitino.utils.PrincipalUtils; +import org.apache.hadoop.conf.Configuration; +import org.apache.hadoop.security.UserGroupInformation; +import org.apache.iceberg.io.FileIO; +import org.apache.iceberg.jdbc.JdbcCatalog; +import org.apache.iceberg.jdbc.JdbcClientPool; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +/** + * ClosableHiveCatalog is a wrapper class to wrap Iceberg HiveCatalog to do some clean-up work like + * closing resources. + */ +public class ClosableJdbcCatalog extends JdbcCatalog implements Closeable, SupportsKerberos { + + private static final Logger LOGGER= LoggerFactory.getLogger(ClosableJdbcCatalog.class); + + private KerberosClient kerberosClient; + + private Configuration hadoopConf; + + public ClosableJdbcCatalog() { + super(); + } + + public ClosableJdbcCatalog( + Function<Map<String, String>, FileIO> ioBuilder, + Function<Map<String, String>, JdbcClientPool> clientPoolBuilder, + boolean initializeCatalogTables) { + super(ioBuilder, clientPoolBuilder, initializeCatalogTables); + } + + /** + * Initialize the ClosableHiveCatalog with the given input name and properties. + * + * <p>Note: This method can only be called once as it will create new client pools. + * + * @param inputName name of the catalog + * @param properties properties for the catalog + */ Review Comment: The JavaDoc comment still refers to "ClosableHiveCatalog" but this method is in the "ClosableJdbcCatalog" class. The documentation should be updated to accurately describe this as initializing ClosableJdbcCatalog. -- 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]
