From 1cd1fc9bb2d2b03678b66a51146ff1a30a157cb8 Mon Sep 17 00:00:00 2001 From: yuqi Date: Tue, 10 Oct 2023 22:11:58 +0800 Subject: [PATCH 01/23] Refactor config --- build.gradle.kts | 8 +- catalogs/catalog-hive/build.gradle.kts | 9 ++ .../catalog/hive/HiveCatalogOperations.java | 13 ++- .../src/main/resources/hive.properties | 11 ++ .../build.gradle.kts | 9 ++ .../iceberg/IcebergCatalogOperations.java | 14 +++ .../resources/lakehouse-iceberg.properties | 8 ++ .../graviton/aux/AuxiliaryServiceManager.java | 7 +- .../graviton/catalog/BaseCatalog.java | 21 ++++ .../graviton/catalog/CatalogManager.java | 105 +++++++++--------- .../graviton/utils/IsolatedClassLoader.java | 65 ++++++++--- .../aux/TestAuxiliaryServiceManager.java | 4 +- core/src/test/resources/test.properties | 4 + .../graviton/server/web/rest/TestCatalog.java | 11 ++ .../{java => }/resources/log4j2.properties | 0 15 files changed, 210 insertions(+), 79 deletions(-) create mode 100644 catalogs/catalog-hive/src/main/resources/hive.properties create mode 100644 catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties create mode 100644 core/src/test/resources/test.properties rename server/src/test/{java => }/resources/log4j2.properties (100%) diff --git a/build.gradle.kts b/build.gradle.kts index e9ac81b9bb7..3ddc28d99ac 100644 --- a/build.gradle.kts +++ b/build.gradle.kts @@ -155,7 +155,7 @@ tasks { val outputDir = projectDir.dir("distribution") val compileDistribution by registering { - dependsOn("copySubprojectDependencies", "copyCatalogLibs", "copySubprojectLib") + dependsOn("copySubprojectDependencies", "copyCatalogLibAndConfigs", "copySubprojectLib") group = "graviton distribution" outputs.dir(projectDir.dir("distribution/package")) @@ -230,9 +230,9 @@ tasks { } } - val copyCatalogLibs by registering(Copy::class) { - dependsOn(":catalogs:catalog-hive:copyCatalogLibs", - ":catalogs:catalog-lakehouse-iceberg:copyCatalogLibs") + val copyCatalogLibAndConfigs by registering(Copy::class) { + dependsOn(":catalogs:catalog-hive:copyLibAndConfig", + ":catalogs:catalog-lakehouse-iceberg:copyLibAndConfig") } clean { diff --git a/catalogs/catalog-hive/build.gradle.kts b/catalogs/catalog-hive/build.gradle.kts index 66b3d0440d9..a02f4a4d0c8 100644 --- a/catalogs/catalog-hive/build.gradle.kts +++ b/catalogs/catalog-hive/build.gradle.kts @@ -90,4 +90,13 @@ tasks { from("build/libs") into("${rootDir}/distribution/package/catalogs/hive/libs") } + + val copyCatalogConfig by registering(Copy::class) { + from("src/main/resources") + into("${rootDir}/distribution/package/catalogs/hive/conf") + } + + val copyLibAndConfig by registering(Copy::class) { + dependsOn(copyCatalogConfig, copyCatalogLibs) + } } diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index 3c0c3724ca3..757bfb90d35 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -67,6 +67,9 @@ public class HiveCatalogOperations implements CatalogOperations, SupportsSchemas private HiveTablePropertiesMetadata tablePropertiesMetadata; + private static final String HIVE_CONFIG_NAME = "hive.properties"; + + public static final String HIVE_RELATED_CONFIG_PREFIX = "graviton.catalog.hive."; /** * Constructs a new instance of HiveCatalogOperations. * @@ -85,7 +88,15 @@ public HiveCatalogOperations(CatalogEntity entity) { @Override public void initialize(Map conf) throws RuntimeException { Configuration hadoopConf = new Configuration(); - conf.forEach(hadoopConf::set); + conf.forEach( + (k, v) -> { + if (k.startsWith(HIVE_RELATED_CONFIG_PREFIX)) { + hadoopConf.set(k.replace(HIVE_RELATED_CONFIG_PREFIX, ""), v); + } else { + hadoopConf.set(k, v); + } + }); + hiveConf = new HiveConf(hadoopConf, HiveCatalogOperations.class); // todo(xun): add hive client pool size in config diff --git a/catalogs/catalog-hive/src/main/resources/hive.properties b/catalogs/catalog-hive/src/main/resources/hive.properties new file mode 100644 index 00000000000..21324a3755b --- /dev/null +++ b/catalogs/catalog-hive/src/main/resources/hive.properties @@ -0,0 +1,11 @@ +# +# Copyright 2023 Datastrato. +# This software is licensed under the Apache License version 2. +# + +## This file holds common properties for hive catalog and metastore, for more, please refer to +## `org.apache.hadoop.hive.conf.HiveConf` + +## If we want to specify Hive catalog-related configuration like 'hive.metastore.client.capability.check', we can do it like this: +## graviton.catalog.hive.hive.metastore.client.capability.check = true, and 'graviton.catalog.hive' is the prefix that mark +## the configuration is for Hive catalog, and 'hive.metastore.client.capability.check' is the configuration key. diff --git a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts index 0bc6ca2c286..237ed5cd865 100644 --- a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts +++ b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts @@ -90,4 +90,13 @@ tasks { from("build/libs") into("${rootDir}/distribution/package/catalogs/lakehouse-iceberg/libs") } + + val copyCatalogConfig by registering(Copy::class) { + from("src/main/resources") + into("${rootDir}/distribution/package/catalogs/lakehouse-iceberg/conf") + } + + val copyLibAndConfig by registering(Copy::class) { + dependsOn(copyCatalogLibs, copyCatalogConfig) + } } diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java index f8cb2fdc367..0429220a840 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java +++ b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java @@ -54,6 +54,8 @@ public class IcebergCatalogOperations implements CatalogOperations, SupportsSche private final CatalogEntity entity; + public static final String ICEBERG_RELATED_CONFIG_PREFIX = "graviton.catalog.iceberg."; + /** * Constructs a new instance of IcebergCatalogOperations. * @@ -71,6 +73,18 @@ public IcebergCatalogOperations(CatalogEntity entity) { */ @Override public void initialize(Map conf) throws RuntimeException { + // Convert Graviton config (May starts with ICEBERG_RELATED_CONFIG_PREFIX) to Iceberg + // configuration + Map trimConfig = Maps.newHashMap(); + conf.forEach( + (k, v) -> { + if (k.startsWith(ICEBERG_RELATED_CONFIG_PREFIX)) { + trimConfig.put(k.replace(ICEBERG_RELATED_CONFIG_PREFIX, ""), v); + } else { + trimConfig.put(k, v); + } + }); + IcebergConfig icebergConfig = new IcebergConfig(); icebergConfig.loadFromMap(conf, k -> true); this.icebergTableOps = new IcebergTableOps(icebergConfig); diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties new file mode 100644 index 00000000000..2ab3b5c5e7f --- /dev/null +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties @@ -0,0 +1,8 @@ +# +# Copyright 2023 Datastrato. +# This software is licensed under the Apache License version 2. +# + +## This file holds common properties for Iceberg catalog. The format of the key is +## 'graviton.catalog.iceberg.{iceberg-inner-config-key}' and `iceberg-inner-config-key` is the +## key in the iceberg catalog configuration file. diff --git a/core/src/main/java/com/datastrato/graviton/aux/AuxiliaryServiceManager.java b/core/src/main/java/com/datastrato/graviton/aux/AuxiliaryServiceManager.java index 0b0aec5884c..3e8865e8ff3 100644 --- a/core/src/main/java/com/datastrato/graviton/aux/AuxiliaryServiceManager.java +++ b/core/src/main/java/com/datastrato/graviton/aux/AuxiliaryServiceManager.java @@ -12,6 +12,7 @@ import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; import com.google.common.collect.Streams; import java.nio.file.Files; import java.nio.file.Path; @@ -79,8 +80,8 @@ public GravitonAuxiliaryService loadAuxService( } @VisibleForTesting - public IsolatedClassLoader getIsolatedClassLoader(String classPath) { - return IsolatedClassLoader.buildClassLoader(classPath); + public IsolatedClassLoader getIsolatedClassLoader(List classPaths) { + return IsolatedClassLoader.buildClassLoader(classPaths); } @VisibleForTesting @@ -115,7 +116,7 @@ private void registerAuxService(String auxServiceName, Map confi classPath = getValidPath(auxServiceName, classPath); LOG.info("AuxService name:{}, config:{}, classpath:{}", auxServiceName, config, classPath); - IsolatedClassLoader isolatedClassLoader = getIsolatedClassLoader(classPath); + IsolatedClassLoader isolatedClassLoader = getIsolatedClassLoader(Lists.newArrayList(classPath)); try { GravitonAuxiliaryService gravitonAuxiliaryService = loadAuxService(auxServiceName, isolatedClassLoader); diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index a25a349052f..9e491f2a7a8 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -10,7 +10,11 @@ import com.datastrato.graviton.meta.CatalogEntity; import com.google.common.base.Preconditions; import com.google.common.collect.Maps; +import java.io.InputStream; import java.util.Map; +import java.util.Properties; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * The abstract base class for Catalog implementations. @@ -27,6 +31,8 @@ public abstract class BaseCatalog implements Catalog, CatalogProvider, HasPropertyMetadata { + private static final Logger LOG = LoggerFactory.getLogger(BaseCatalog.class); + private CatalogEntity entity; private Map conf; @@ -42,6 +48,21 @@ public abstract class BaseCatalog */ protected abstract CatalogOperations newOps(Map config); + /** Get all configuration properties for the catalog in the classpath. */ + public Map loadCatalogSpecificProperties(String catalogName) { + Map configMap = Maps.newHashMap(); + try { + InputStream inputStream = this.getClass().getClassLoader().getResourceAsStream(catalogName); + Properties properties = new Properties(); + properties.load(inputStream); + properties.forEach((key, value) -> configMap.put(key.toString(), value.toString())); + } catch (Exception e) { + // If the catalog-specific configuration file is not found, it will not be loaded. + LOG.warn("Failed to load catalog specific configurations", e); + } + return configMap; + } + @Override public PropertiesMetadata tablePropertiesMetadata() throws UnsupportedOperationException { return ops().tablePropertiesMetadata(); diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index d414c8c732f..c773f3fc7ee 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -4,6 +4,8 @@ */ package com.datastrato.graviton.catalog; +import static com.datastrato.graviton.StringIdentifier.ID_KEY; + import com.datastrato.graviton.Catalog; import com.datastrato.graviton.CatalogChange; import com.datastrato.graviton.CatalogChange.RemoveProperty; @@ -34,7 +36,9 @@ import com.github.benmanes.caffeine.cache.Scheduler; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; +import com.google.common.collect.ImmutableMap; import com.google.common.collect.Iterables; +import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Streams; import com.google.common.util.concurrent.ThreadFactoryBuilder; @@ -214,8 +218,7 @@ public NameIdentifier[] listCatalogs(Namespace namespace) throws NoSuchMetalakeE */ @Override public Catalog loadCatalog(NameIdentifier ident) throws NoSuchCatalogException { - CatalogWrapper wrapper = loadCatalogInternal(ident); - return wrapper.catalog; + return loadCatalogAndWrap(ident).catalog; } /** @@ -272,11 +275,6 @@ public Catalog createCatalog( return e; }); CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(entity)); - wrapper.doWithPropertiesMeta( - f -> { - f.catalogPropertiesMetadata().validatePropertyForCreate(properties); - return null; - }); return wrapper.catalog; } catch (EntityAlreadyExistsException e1) { LOG.warn("Catalog {} already exists", ident, e1); @@ -446,13 +444,15 @@ private CatalogWrapper loadCatalogInternal(NameIdentifier ident) throws NoSuchCa private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { Map mergedConf = - mergeConf(entity.getProperties(), catalogConf(entity.name(), config)); + entity.getProperties() == null + ? ImmutableMap.of() + : ImmutableMap.copyOf(entity.getProperties()); String provider = entity.getProvider(); IsolatedClassLoader classLoader; if (config.get(Configs.CATALOG_LOAD_ISOLATED)) { - String pkgPath = buildPkgPath(mergedConf, provider); - classLoader = IsolatedClassLoader.buildClassLoader(pkgPath); + List pkgPaths = buildPkgPaths(mergedConf, provider); + classLoader = IsolatedClassLoader.buildClassLoader(pkgPaths); } else { // This will use the current class loader, it is mainly used for test. classLoader = @@ -484,25 +484,36 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { throw new RuntimeException("Failed to load catalog with provider: " + provider); } - // Initialize the catalog - catalog = catalog.withCatalogEntity(entity).withCatalogConf(mergedConf); + // Load catalog specific properties CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); + Map catalogSpecificConfig = + classLoader.withClassLoader( + cl -> catalog.loadCatalogSpecificProperties(provider + ".properties"), + RuntimeException.class); + + Map totalMergedConf = + mergeConf(Maps.newHashMap(mergedConf), catalogSpecificConfig); + // Initialize the catalog + catalog.withCatalogConf(totalMergedConf).withCatalogEntity(entity); + + // Validate catalog properties and initialize the config + classLoader.withClassLoader( + cl -> { + Map configWithoutId = Maps.newHashMap(totalMergedConf); + configWithoutId.remove(ID_KEY); + catalog.ops().catalogPropertiesMetadata().validatePropertyForCreate(configWithoutId); + + // Call wrapper.catalog.properties() to make BaseCatalog#properties in IsolatedClassLoader + // not null. Why we do this? Because wrapper.catalog.properties() need to be called in the + // IsolatedClassLoader, it needs to load the specific catalog class such as HiveCatalog or + // so. + // For simply, We will preload the value of properties and thus AppClassLoader can get the + // value of properties. + wrapper.catalog.properties(); + return null; + }, + IllegalArgumentException.class); - // Call wrapper.catalog.properties() to make BaseCatalog#properties in IsolatedClassLoader - // not null. Why we do this? Because wrapper.catalog.properties() need to be called in the - // IsolatedClassLoader, it needs to load the specific catalog class such as HiveCatalog or so. - // For simply, We will preload the value of properties and thus AppClassLoader can get the - // value of properties. - try { - wrapper.doWithPropertiesMeta( - f -> { - wrapper.catalog.properties(); - return null; - }); - } catch (Exception e) { - LOG.error("Failed to load catalog '{}' properties", entity.name(), e); - throw new RuntimeException(e); - } return wrapper; } @@ -512,12 +523,17 @@ static Map mergeConf(Map properties, Map catalogConf(String name, Config config) { String confPrefix = "graviton.catalog." + name + "."; return config.getConfigsWithPrefix(confPrefix); } - private String buildPkgPath(Map conf, String provider) { + private List buildPkgPaths(Map conf, String provider) { String pkg = conf.get(Catalog.PROPERTY_PACKAGE); String gravitonHome = System.getenv("GRAVITON_HOME"); @@ -527,32 +543,17 @@ private String buildPkgPath(Map conf, String provider) { String pkgPath; if (pkg != null) { pkgPath = pkg; + return Lists.newArrayList(pkgPath.split(",")); } else if (!testEnv) { - pkgPath = - gravitonHome - + File.separator - + "catalogs" - + File.separator - + provider - + File.separator - + "libs"; - } else { - pkgPath = - new StringBuilder() - .append(gravitonHome) - .append(File.separator) - .append("catalogs") - .append(File.separator) - .append("catalog-") - .append(provider) - .append(File.separator) - .append("build") - .append(File.separator) - .append("libs") - .toString(); + pkgPath = String.join(File.separator, gravitonHome, "catalogs", provider); + // Add the config and lib to the classpath. + return Lists.newArrayList( + pkgPath + File.separator + "conf", pkgPath + File.separator + "libs"); } - - return pkgPath; + pkgPath = String.join(File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build"); + // Add the config and lib to the classpath. + return Lists.newArrayList( + pkgPath + File.separator + "resources", pkgPath + File.separator + "libs"); } private Class lookupCatalogProvider(String provider, ClassLoader cl) { diff --git a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java index 094ce1d4fc4..fc1e2ddf1d9 100644 --- a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java +++ b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java @@ -72,26 +72,57 @@ public T withClassLoader(ThrowableFunction fn) throws Except } } - public static IsolatedClassLoader buildClassLoader(String pkgPath) { - // Listing all the jars under the package path and build the isolated class loader. - File pkgFolder = new File(pkgPath); - if (!pkgFolder.exists() - || !pkgFolder.isDirectory() - || !pkgFolder.canRead() - || !pkgFolder.canExecute()) { - throw new IllegalArgumentException("Invalid package path: " + pkgPath); + /** + * Executes the provided function within the isolated class loading context and wraps any + * exception, for more, please refer to {@link #withClassLoader(ThrowableFunction)}. + */ + public T withClassLoader( + ThrowableFunction fn, Class exceptionClass) { + try { + return withClassLoader(fn); + } catch (Exception e) { + if (exceptionClass.isInstance(e)) { + throw (E) e; + } + throw new RuntimeException(e); } + } + + public static IsolatedClassLoader buildClassLoader(List pkgPaths) { + // Listing all the jars under the package path and build the isolated class loader. List jars = Lists.newArrayList(); - Arrays.stream(pkgFolder.listFiles()) - .forEach( - f -> { - try { - jars.add(f.toURI().toURL()); - } catch (MalformedURLException e) { - LOG.warn("Failed to read jar file: {}", f.getAbsolutePath(), e); - } - }); + for (String path : pkgPaths) { + File pkgFolder = new File(path); + if (!pkgFolder.exists() + || !pkgFolder.isDirectory() + || !pkgFolder.canRead() + || !pkgFolder.canExecute()) { + throw new IllegalArgumentException("Invalid package path: " + pkgPaths); + } + + // Add all the jars under the package path. + Arrays.stream(pkgFolder.listFiles()) + .forEach( + f -> { + try { + jars.add(f.toURI().toURL()); + } catch (MalformedURLException e) { + LOG.warn("Failed to read jar file: {}", f.getAbsolutePath(), e); + } + }); + + // Add itself to the classpath. + Lists.newArrayList(pkgFolder).stream() + .forEach( + f -> { + try { + jars.add(f.toURI().toURL()); + } catch (MalformedURLException e) { + LOG.warn("Failed to read jar file: {}", f.getAbsolutePath(), e); + } + }); + } return new IsolatedClassLoader(jars, Collections.emptyList(), Collections.emptyList()); } diff --git a/core/src/test/java/com/datastrato/graviton/aux/TestAuxiliaryServiceManager.java b/core/src/test/java/com/datastrato/graviton/aux/TestAuxiliaryServiceManager.java index b87450d542a..6aa8473ca7d 100644 --- a/core/src/test/java/com/datastrato/graviton/aux/TestAuxiliaryServiceManager.java +++ b/core/src/test/java/com/datastrato/graviton/aux/TestAuxiliaryServiceManager.java @@ -6,7 +6,7 @@ package com.datastrato.graviton.aux; import static org.mockito.ArgumentMatchers.any; -import static org.mockito.ArgumentMatchers.anyString; +import static org.mockito.ArgumentMatchers.anyList; import static org.mockito.Mockito.doReturn; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.spy; @@ -51,7 +51,7 @@ public void testGravitonAuxServiceManager() throws Exception { AuxiliaryServiceManager auxServiceManager = new AuxiliaryServiceManager(); AuxiliaryServiceManager spyAuxManager = spy(auxServiceManager); - doReturn(isolatedClassLoader).when(spyAuxManager).getIsolatedClassLoader(anyString()); + doReturn(isolatedClassLoader).when(spyAuxManager).getIsolatedClassLoader(anyList()); doReturn(auxService).when(spyAuxManager).loadAuxService("mock1", isolatedClassLoader); doReturn(auxService2).when(spyAuxManager).loadAuxService("mock2", isolatedClassLoader); diff --git a/core/src/test/resources/test.properties b/core/src/test/resources/test.properties new file mode 100644 index 00000000000..0f778e5b640 --- /dev/null +++ b/core/src/test/resources/test.properties @@ -0,0 +1,4 @@ +# +# Copyright 2023 Datastrato. +# This software is licensed under the Apache License version 2. +# \ No newline at end of file diff --git a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java index b6825f67252..db0fb9abc9f 100644 --- a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java +++ b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java @@ -10,6 +10,8 @@ import com.google.common.collect.Maps; import java.io.IOException; import java.util.Map; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; public class TestCatalog extends BaseCatalog { @Override @@ -36,4 +38,13 @@ public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperatio } }; } + + @Test + void test() { + TestCatalog testCatalog = new TestCatalog(); + // This will load the log4j2.xml file from the classpath + Map configs = testCatalog.loadCatalogSpecificProperties("log4j2.properties"); + Assertions.assertTrue(configs.containsKey("status")); + Assertions.assertEquals("warn", configs.get("status")); + } } diff --git a/server/src/test/java/resources/log4j2.properties b/server/src/test/resources/log4j2.properties similarity index 100% rename from server/src/test/java/resources/log4j2.properties rename to server/src/test/resources/log4j2.properties From 37c7947308d14cd764236c0bea11e023c1aa73f4 Mon Sep 17 00:00:00 2001 From: yuqi Date: Wed, 11 Oct 2023 16:16:00 +0800 Subject: [PATCH 02/23] Changes according to discussion --- .../catalog/hive/HiveCatalogOperations.java | 10 +-- .../src/main/resources/hive.properties | 5 +- .../iceberg/IcebergCatalogOperations.java | 12 --- .../resources/lakehouse-iceberg.properties | 2 +- .../graviton/catalog/BaseCatalog.java | 4 +- .../graviton/catalog/CatalogManager.java | 85 ++++++++++++------- .../graviton/meta/CatalogEntity.java | 15 ++++ .../graviton/catalog/TestCatalogManager.java | 21 +++-- .../test/catalog/hive/CatalogHiveIT.java | 21 ++++- .../graviton/server/web/rest/TestCatalog.java | 2 +- 10 files changed, 107 insertions(+), 70 deletions(-) diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index 73543c3f2bc..c978f785875 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -90,15 +90,7 @@ public HiveCatalogOperations(CatalogEntity entity) { @Override public void initialize(Map conf) throws RuntimeException { Configuration hadoopConf = new Configuration(); - conf.forEach( - (k, v) -> { - if (k.startsWith(HIVE_RELATED_CONFIG_PREFIX)) { - hadoopConf.set(k.replace(HIVE_RELATED_CONFIG_PREFIX, ""), v); - } else { - hadoopConf.set(k, v); - } - }); - + conf.forEach(hadoopConf::set); hiveConf = new HiveConf(hadoopConf, HiveCatalogOperations.class); // todo(xun): add hive client pool size in config diff --git a/catalogs/catalog-hive/src/main/resources/hive.properties b/catalogs/catalog-hive/src/main/resources/hive.properties index 21324a3755b..dee575a506d 100644 --- a/catalogs/catalog-hive/src/main/resources/hive.properties +++ b/catalogs/catalog-hive/src/main/resources/hive.properties @@ -7,5 +7,6 @@ ## `org.apache.hadoop.hive.conf.HiveConf` ## If we want to specify Hive catalog-related configuration like 'hive.metastore.client.capability.check', we can do it like this: -## graviton.catalog.hive.hive.metastore.client.capability.check = true, and 'graviton.catalog.hive' is the prefix that mark -## the configuration is for Hive catalog, and 'hive.metastore.client.capability.check' is the configuration key. +## graviton.bypass.hive.metastore.client.capability.check = true, and 'graviton.bypass' is the prefix that +## the configuration will be directly by pass to backend engine, and 'hive.metastore.client.capability.check' is the configuration key. +graviton.bypass.hive.metastore.client.capability.check = false \ No newline at end of file diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java index b24904e6794..10d6963e3e3 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java +++ b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java @@ -84,18 +84,6 @@ public IcebergCatalogOperations(CatalogEntity entity) { */ @Override public void initialize(Map conf) throws RuntimeException { - // Convert Graviton config (May starts with ICEBERG_RELATED_CONFIG_PREFIX) to Iceberg - // configuration - Map trimConfig = Maps.newHashMap(); - conf.forEach( - (k, v) -> { - if (k.startsWith(ICEBERG_RELATED_CONFIG_PREFIX)) { - trimConfig.put(k.replace(ICEBERG_RELATED_CONFIG_PREFIX, ""), v); - } else { - trimConfig.put(k, v); - } - }); - IcebergConfig icebergConfig = new IcebergConfig(); icebergConfig.loadFromMap(conf, k -> true); this.icebergTableOps = new IcebergTableOps(icebergConfig); diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties index 2ab3b5c5e7f..2ea6df5dbd7 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties @@ -4,5 +4,5 @@ # ## This file holds common properties for Iceberg catalog. The format of the key is -## 'graviton.catalog.iceberg.{iceberg-inner-config-key}' and `iceberg-inner-config-key` is the +## 'graviton.bypass.{iceberg-inner-config-key}' and `iceberg-inner-config-key` is the ## key in the iceberg catalog configuration file. diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index 9e491f2a7a8..5c3b7d47603 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -49,10 +49,10 @@ public abstract class BaseCatalog protected abstract CatalogOperations newOps(Map config); /** Get all configuration properties for the catalog in the classpath. */ - public Map loadCatalogSpecificProperties(String catalogName) { + public Map loadCatalogSpecificProperties(String fileURL) { Map configMap = Maps.newHashMap(); try { - InputStream inputStream = this.getClass().getClassLoader().getResourceAsStream(catalogName); + InputStream inputStream = this.getClass().getClassLoader().getResourceAsStream(fileURL); Properties properties = new Properties(); properties.load(inputStream); properties.forEach((key, value) -> configMap.put(key.toString(), value.toString())); diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index c773f3fc7ee..e1bcf5ee521 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -64,6 +64,12 @@ public class CatalogManager implements SupportsCatalogs, Closeable { private static final Logger LOG = LoggerFactory.getLogger(CatalogManager.class); + // Any graviton configuration that starts with this prefix will be trim and passed to the specific + // catalog + // implementation. For example, if the configuration is "graviton.bypass.hive.metastore.uris", + // then we will + // trim the prefix and pass "hive.metastore.uris" to the hive catalog implementation. + public static final String CATALOG_BYPASS_PREFIX = "graviton.bypass."; /** Wrapper class for a catalog instance and its class loader. */ public static class CatalogWrapper { @@ -241,48 +247,47 @@ public Catalog createCatalog( String comment, Map properties) throws NoSuchMetalakeException, CatalogAlreadyExistsException { - try { - CatalogEntity entity = - store.executeInTransaction( - () -> { - NameIdentifier metalakeIdent = NameIdentifier.of(ident.namespace().levels()); - if (!store.exists(metalakeIdent, EntityType.METALAKE)) { - LOG.warn("Metalake {} does not exist", metalakeIdent); - throw new NoSuchMetalakeException( - "Metalake " + metalakeIdent + " does not exist"); - } - - long uid = idGenerator.nextId(); - StringIdentifier stringId = StringIdentifier.fromId(uid); + long uid = idGenerator.nextId(); + StringIdentifier stringId = StringIdentifier.fromId(uid); + CatalogEntity e = + new CatalogEntity.Builder() + .withId(uid) + .withName(ident.name()) + .withNamespace(ident.namespace()) + .withType(type) + .withProvider(provider) + .withComment(comment) + .withProperties(StringIdentifier.addToProperties(stringId, properties)) + .withAuditInfo( + new AuditInfo.Builder() + .withCreator("graviton") /* TODO. Should change to real user */ + .withCreateTime(Instant.now()) + .build()) + .build(); - CatalogEntity e = - new CatalogEntity.Builder() - .withId(uid) - .withName(ident.name()) - .withNamespace(ident.namespace()) - .withType(type) - .withProvider(provider) - .withComment(comment) - .withProperties(StringIdentifier.addToProperties(stringId, properties)) - .withAuditInfo( - new AuditInfo.Builder() - .withCreator("graviton") /* TODO. Should change to real user */ - .withCreateTime(Instant.now()) - .build()) - .build(); + try { + CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(e)); + store.executeInTransaction( + () -> { + NameIdentifier metalakeIdent = NameIdentifier.of(ident.namespace().levels()); + if (!store.exists(metalakeIdent, EntityType.METALAKE)) { + LOG.warn("Metalake {} does not exist", metalakeIdent); + throw new NoSuchMetalakeException("Metalake " + metalakeIdent + " does not exist"); + } - store.put(e, false /* overwrite */); - return e; - }); - CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(entity)); + store.put(e, false /* overwrite */); + return null; + }); return wrapper.catalog; } catch (EntityAlreadyExistsException e1) { LOG.warn("Catalog {} already exists", ident, e1); throw new CatalogAlreadyExistsException("Catalog " + ident + " already exists"); } catch (IllegalArgumentException | NoSuchMetalakeException e2) { + catalogCache.invalidate(ident); throw e2; } catch (Exception e3) { LOG.error("Failed to create catalog {}", ident, e3); + catalogCache.invalidate(ident); throw new RuntimeException(e3); } } @@ -491,10 +496,13 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { cl -> catalog.loadCatalogSpecificProperties(provider + ".properties"), RuntimeException.class); + entity.addProperty(catalogSpecificConfig); + Map totalMergedConf = mergeConf(Maps.newHashMap(mergedConf), catalogSpecificConfig); + Map bypassConfig = getBypassConfig(totalMergedConf); // Initialize the catalog - catalog.withCatalogConf(totalMergedConf).withCatalogEntity(entity); + catalog.withCatalogConf(bypassConfig).withCatalogEntity(entity); // Validate catalog properties and initialize the config classLoader.withClassLoader( @@ -517,6 +525,17 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { return wrapper; } + private Map getBypassConfig(Map totalMergedConf) { + Map bypassConfig = Maps.newHashMap(); + totalMergedConf.forEach( + (key, value) -> { + if (key.startsWith(CATALOG_BYPASS_PREFIX)) { + bypassConfig.put(key.substring(CATALOG_BYPASS_PREFIX.length()), value); + } + }); + return bypassConfig; + } + static Map mergeConf(Map properties, Map conf) { Map mergedConf = conf != null ? Maps.newHashMap(conf) : Maps.newHashMap(); Optional.ofNullable(properties).ifPresent(mergedConf::putAll); diff --git a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java index f257b2262a2..602dc67582c 100644 --- a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java +++ b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java @@ -12,6 +12,8 @@ import com.datastrato.graviton.HasIdentifier; import com.datastrato.graviton.Namespace; import com.google.common.base.Objects; +import com.google.common.collect.ImmutableMap; +import com.google.common.collect.Maps; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -72,6 +74,19 @@ public Map fields() { return Collections.unmodifiableMap(fields); } + public void addProperty(Map extraProperty) { + if (extraProperty == null) { + return; + } + + if (properties == null) { + properties = Maps.newHashMap(extraProperty); + } else { + extraProperty.putAll(properties); + properties = ImmutableMap.copyOf(extraProperty); + } + } + /** * The audit information of the catalog. * diff --git a/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java index 470b1b2bd9f..ab123cce881 100644 --- a/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java +++ b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java @@ -106,7 +106,7 @@ void testLoadTable() throws IOException { .put("key2", "value2") .put("key1", "value1") .put("hidden_key", "hidden_value") - .put("mock", "mock") + .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") .build(); Assertions.assertDoesNotThrow( () -> @@ -137,7 +137,7 @@ void testPropertyValidationInAlter() throws IOException { ImmutableMap.builder() .put("key2", "value2") .put("key1", "value1") - .put("mock", "mock") + .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") .build(); Assertions.assertDoesNotThrow( () -> @@ -152,7 +152,7 @@ void testPropertyValidationInAlter() throws IOException { .put("key1", "value1") .put("key3", "3") .put("key4", "value4") - .put("mock", "mock") + .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") .build(); Assertions.assertDoesNotThrow( () -> @@ -201,7 +201,10 @@ void testPropertyValidationInCreate() throws IOException { // key1 is required; Map props1 = - ImmutableMap.builder().put("key2", "value2").put("mock", "mock").build(); + ImmutableMap.builder() + .put("key2", "value2") + .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") + .build(); IllegalArgumentException e1 = Assertions.assertThrows( IllegalArgumentException.class, @@ -215,7 +218,10 @@ void testPropertyValidationInCreate() throws IOException { // key2 is required; Map props2 = - ImmutableMap.builder().put("key1", "value1").put("mock", "mock").build(); + ImmutableMap.builder() + .put("key1", "value1") + .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") + .build(); e1 = Assertions.assertThrows( IllegalArgumentException.class, @@ -232,7 +238,7 @@ void testPropertyValidationInCreate() throws IOException { .put("key1", "value1") .put("key2", "value2") .put("key3", "a12a1a") - .put("mock", "mock") + .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") .build(); e1 = Assertions.assertThrows( @@ -256,6 +262,8 @@ public void testCreateCatalog() { testProperties(props, testCatalog.properties()); Assertions.assertEquals(Catalog.Type.RELATIONAL, testCatalog.type()); + Assertions.assertNotNull(catalogManager.catalogCache.getIfPresent(ident)); + // Test create under non-existed metalake NameIdentifier ident2 = NameIdentifier.of("metalake1", "test1"); Throwable exception1 = @@ -265,6 +273,7 @@ public void testCreateCatalog() { catalogManager.createCatalog( ident2, Catalog.Type.RELATIONAL, provider, "comment", props)); Assertions.assertTrue(exception1.getMessage().contains("Metalake metalake1 does not exist")); + Assertions.assertNull(catalogManager.catalogCache.getIfPresent(ident2)); // Test create with duplicated name Throwable exception2 = diff --git a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java index f30792c6d93..0232a1f4d8c 100644 --- a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java +++ b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java @@ -4,6 +4,7 @@ */ package com.datastrato.graviton.integration.test.catalog.hive; +import static com.datastrato.graviton.catalog.CatalogManager.CATALOG_BYPASS_PREFIX; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.COMMENT; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.EXTERNAL; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.FORMAT; @@ -127,10 +128,15 @@ private static void createMetalake() { private static void createCatalog() { Map properties = Maps.newHashMap(); - properties.put(HiveConf.ConfVars.METASTOREURIS.varname, HIVE_METASTORE_URIS); - properties.put(HiveConf.ConfVars.METASTORETHRIFTCONNECTIONRETRIES.varname, "30"); - properties.put(HiveConf.ConfVars.METASTORETHRIFTFAILURERETRIES.varname, "30"); - properties.put(HiveConf.ConfVars.METASTORE_CLIENT_CONNECT_RETRY_DELAY.varname, "5"); + properties.put( + CATALOG_BYPASS_PREFIX + HiveConf.ConfVars.METASTOREURIS.varname, HIVE_METASTORE_URIS); + properties.put( + CATALOG_BYPASS_PREFIX + HiveConf.ConfVars.METASTORETHRIFTCONNECTIONRETRIES.varname, "30"); + properties.put( + CATALOG_BYPASS_PREFIX + HiveConf.ConfVars.METASTORETHRIFTFAILURERETRIES.varname, "30"); + properties.put( + CATALOG_BYPASS_PREFIX + HiveConf.ConfVars.METASTORE_CLIENT_CONNECT_RETRY_DELAY.varname, + "5"); Catalog createdCatalog = metalake.createCatalog( @@ -140,6 +146,13 @@ private static void createCatalog() { "comment", properties); Catalog loadCatalog = metalake.loadCatalog(NameIdentifier.of(metalakeName, catalogName)); + Assertions.assertTrue( + loadCatalog + .properties() + .containsKey("graviton.bypass.hive.metastore.client.capability.check")); + Assertions.assertEquals( + "false", + loadCatalog.properties().get("graviton.bypass.hive.metastore.client.capability.check")); Assertions.assertEquals(createdCatalog, loadCatalog); catalog = loadCatalog; diff --git a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java index db0fb9abc9f..09b43e4fe13 100644 --- a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java +++ b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java @@ -42,7 +42,7 @@ public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperatio @Test void test() { TestCatalog testCatalog = new TestCatalog(); - // This will load the log4j2.xml file from the classpath + // This will load the log4j2.properties file from the classpath Map configs = testCatalog.loadCatalogSpecificProperties("log4j2.properties"); Assertions.assertTrue(configs.containsKey("status")); Assertions.assertEquals("warn", configs.get("status")); From 1d051a37abe528d97752a1a08549626128d747dc Mon Sep 17 00:00:00 2001 From: yuqi Date: Wed, 11 Oct 2023 16:23:42 +0800 Subject: [PATCH 03/23] Remove unused code and add some description --- .../graviton/catalog/hive/HiveCatalogOperations.java | 3 --- .../catalog/lakehouse/iceberg/IcebergCatalogOperations.java | 1 - .../main/java/com/datastrato/graviton/catalog/BaseCatalog.java | 3 ++- .../graviton/integration/test/catalog/hive/CatalogHiveIT.java | 3 +++ 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index c978f785875..2d32f07d8bd 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -69,9 +69,6 @@ public class HiveCatalogOperations implements CatalogOperations, SupportsSchemas private HiveTablePropertiesMetadata tablePropertiesMetadata; - private static final String HIVE_CONFIG_NAME = "hive.properties"; - - public static final String HIVE_RELATED_CONFIG_PREFIX = "graviton.catalog.hive."; /** * Constructs a new instance of HiveCatalogOperations. * diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java index 10d6963e3e3..6c2bfdd0bec 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java +++ b/catalogs/catalog-lakehouse-iceberg/src/main/java/com/datastrato/graviton/catalog/lakehouse/iceberg/IcebergCatalogOperations.java @@ -64,7 +64,6 @@ public class IcebergCatalogOperations implements CatalogOperations, SupportsSche private final CatalogEntity entity; - public static final String ICEBERG_RELATED_CONFIG_PREFIX = "graviton.catalog.iceberg."; private IcebergTableOpsHelper icebergTableOpsHelper; /** diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index 5c3b7d47603..5cf79d883f1 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -48,7 +48,7 @@ public abstract class BaseCatalog */ protected abstract CatalogOperations newOps(Map config); - /** Get all configuration properties for the catalog in the classpath. */ + /** Load properties for the catalog from file in the classpath. */ public Map loadCatalogSpecificProperties(String fileURL) { Map configMap = Maps.newHashMap(); try { @@ -58,6 +58,7 @@ public Map loadCatalogSpecificProperties(String fileURL) { properties.forEach((key, value) -> configMap.put(key.toString(), value.toString())); } catch (Exception e) { // If the catalog-specific configuration file is not found, it will not be loaded. + // Should we throw exception directly? LOG.warn("Failed to load catalog specific configurations", e); } return configMap; diff --git a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java index 0232a1f4d8c..2a7418c28a1 100644 --- a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java +++ b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java @@ -146,6 +146,9 @@ private static void createCatalog() { "comment", properties); Catalog loadCatalog = metalake.loadCatalog(NameIdentifier.of(metalakeName, catalogName)); + + // We have added the bypass property 'graviton.bypass.hive.metastore.client.capability.check' + // to the hive.properties, so we need to check if the property has been added successfully. Assertions.assertTrue( loadCatalog .properties() From 109a2ad2cb9719ba1ae5d308091eaaef759409b3 Mon Sep 17 00:00:00 2001 From: yuqi Date: Wed, 11 Oct 2023 16:28:13 +0800 Subject: [PATCH 04/23] Add some description --- .../datastrato/graviton/catalog/CatalogManager.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index e1bcf5ee521..00055033f07 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -65,10 +65,9 @@ public class CatalogManager implements SupportsCatalogs, Closeable { private static final Logger LOG = LoggerFactory.getLogger(CatalogManager.class); // Any graviton configuration that starts with this prefix will be trim and passed to the specific - // catalog - // implementation. For example, if the configuration is "graviton.bypass.hive.metastore.uris", - // then we will - // trim the prefix and pass "hive.metastore.uris" to the hive catalog implementation. + // catalog implementation. For example, if the configuration is + // "graviton.bypass.hive.metastore.uris", + // then we will trim the prefix and pass "hive.metastore.uris" to the hive catalog implementation. public static final String CATALOG_BYPASS_PREFIX = "graviton.bypass."; /** Wrapper class for a catalog instance and its class loader. */ @@ -266,6 +265,9 @@ public Catalog createCatalog( .build(); try { + // We need to create the catalog wrapper before we put the entity into the store, because we + // need + // to load the catalog-specific properties from the catalog-specific config file. CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(e)); store.executeInTransaction( () -> { From 805d511a9ed3e6de42f50a969a5cd1b926858cf7 Mon Sep 17 00:00:00 2001 From: yuqi Date: Wed, 11 Oct 2023 17:45:22 +0800 Subject: [PATCH 05/23] Fix some minor issues --- .../graviton/catalog/BaseCatalog.java | 12 ++- .../graviton/catalog/CatalogManager.java | 74 ++++++++++--------- .../graviton/meta/CatalogEntity.java | 6 +- 3 files changed, 54 insertions(+), 38 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index 5cf79d883f1..36aee2f7294 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -9,6 +9,7 @@ import com.datastrato.graviton.CatalogProvider; import com.datastrato.graviton.meta.CatalogEntity; import com.google.common.base.Preconditions; +import com.google.common.base.Throwables; import com.google.common.collect.Maps; import java.io.InputStream; import java.util.Map; @@ -53,13 +54,16 @@ public Map loadCatalogSpecificProperties(String fileURL) { Map configMap = Maps.newHashMap(); try { InputStream inputStream = this.getClass().getClassLoader().getResourceAsStream(fileURL); - Properties properties = new Properties(); - properties.load(inputStream); - properties.forEach((key, value) -> configMap.put(key.toString(), value.toString())); + Properties loadProperties = new Properties(); + loadProperties.load(inputStream); + loadProperties.forEach((key, value) -> configMap.put(key.toString(), value.toString())); } catch (Exception e) { // If the catalog-specific configuration file is not found, it will not be loaded. // Should we throw exception directly? - LOG.warn("Failed to load catalog specific configurations", e); + LOG.warn( + "Failed to load catalog specific configurations, file name: '{}', Exception:\n{}", + fileURL, + Throwables.getStackTraceAsString(e)); } return configMap; } diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 00055033f07..2128202d5a4 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -266,8 +266,7 @@ public Catalog createCatalog( try { // We need to create the catalog wrapper before we put the entity into the store, because we - // need - // to load the catalog-specific properties from the catalog-specific config file. + // need to load the catalog-specific properties from the catalog-specific config file. CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(e)); store.executeInTransaction( () -> { @@ -468,28 +467,7 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { } // Load Catalog class instance - BaseCatalog catalog; - try { - catalog = - classLoader.withClassLoader( - cl -> { - try { - Class providerClz = - lookupCatalogProvider(provider, cl); - return (BaseCatalog) providerClz.newInstance(); - } catch (Exception e) { - LOG.error("Failed to load catalog with provider: {}", provider, e); - throw new RuntimeException(e); - } - }); - } catch (Exception e) { - LOG.error("Failed to load catalog with class loader", e); - throw new RuntimeException(e); - } - - if (catalog == null) { - throw new RuntimeException("Failed to load catalog with provider: " + provider); - } + BaseCatalog catalog = createCatalogInstance(classLoader, provider); // Load catalog specific properties CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); @@ -498,7 +476,9 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { cl -> catalog.loadCatalogSpecificProperties(provider + ".properties"), RuntimeException.class); - entity.addProperty(catalogSpecificConfig); + // Merge catalog-specific configurations to the entity properties, as the entity only contains + // properties that are set by user currently. + entity.mergeProperty(catalogSpecificConfig); Map totalMergedConf = mergeConf(Maps.newHashMap(mergedConf), catalogSpecificConfig); @@ -527,6 +507,32 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { return wrapper; } + private BaseCatalog createCatalogInstance(IsolatedClassLoader classLoader, String provider) { + BaseCatalog catalog; + try { + catalog = + classLoader.withClassLoader( + cl -> { + try { + Class providerClz = + lookupCatalogProvider(provider, cl); + return (BaseCatalog) providerClz.newInstance(); + } catch (Exception e) { + LOG.error("Failed to load catalog with provider: {}", provider, e); + throw new RuntimeException(e); + } + }); + } catch (Exception e) { + LOG.error("Failed to load catalog with class loader", e); + throw new RuntimeException(e); + } + + if (catalog == null) { + throw new RuntimeException("Failed to load catalog with provider: " + provider); + } + return catalog; + } + private Map getBypassConfig(Map totalMergedConf) { Map bypassConfig = Maps.newHashMap(); totalMergedConf.forEach( @@ -563,18 +569,20 @@ private List buildPkgPaths(Map conf, String provider) { String pkgPath; if (pkg != null) { - pkgPath = pkg; - return Lists.newArrayList(pkgPath.split(",")); - } else if (!testEnv) { - pkgPath = String.join(File.separator, gravitonHome, "catalogs", provider); + return Lists.newArrayList(pkg.split(",")); + } else if (testEnv) { + // In test, the catalog package is under the build directory. + pkgPath = String.join(File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build"); // Add the config and lib to the classpath. return Lists.newArrayList( - pkgPath + File.separator + "conf", pkgPath + File.separator + "libs"); + pkgPath + File.separator + "resources", pkgPath + File.separator + "libs"); } - pkgPath = String.join(File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build"); - // Add the config and lib to the classpath. + + // In real environment, the catalog package is under the catalog directory. + pkgPath = String.join(File.separator, gravitonHome, "catalogs", provider); return Lists.newArrayList( - pkgPath + File.separator + "resources", pkgPath + File.separator + "libs"); + pkgPath + File.separator + "conf", pkgPath + File.separator + "libs"); + } private Class lookupCatalogProvider(String provider, ClassLoader cl) { diff --git a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java index 602dc67582c..901c3eccadf 100644 --- a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java +++ b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java @@ -74,7 +74,11 @@ public Map fields() { return Collections.unmodifiableMap(fields); } - public void addProperty(Map extraProperty) { + /** + * Merge the properties of the catalog with the given properties. + * Note: Properties in catalog entity has higher priority than the given properties. + */ + public void mergeProperty(Map extraProperty) { if (extraProperty == null) { return; } From 2de0e664c052ab8329805db98694483b1fd95b9e Mon Sep 17 00:00:00 2001 From: yuqi Date: Wed, 11 Oct 2023 17:48:51 +0800 Subject: [PATCH 06/23] Format code --- .../com/datastrato/graviton/catalog/CatalogManager.java | 7 +++---- .../java/com/datastrato/graviton/meta/CatalogEntity.java | 4 ++-- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 2128202d5a4..41874911ebc 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -572,7 +572,8 @@ private List buildPkgPaths(Map conf, String provider) { return Lists.newArrayList(pkg.split(",")); } else if (testEnv) { // In test, the catalog package is under the build directory. - pkgPath = String.join(File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build"); + pkgPath = + String.join(File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build"); // Add the config and lib to the classpath. return Lists.newArrayList( pkgPath + File.separator + "resources", pkgPath + File.separator + "libs"); @@ -580,9 +581,7 @@ private List buildPkgPaths(Map conf, String provider) { // In real environment, the catalog package is under the catalog directory. pkgPath = String.join(File.separator, gravitonHome, "catalogs", provider); - return Lists.newArrayList( - pkgPath + File.separator + "conf", pkgPath + File.separator + "libs"); - + return Lists.newArrayList(pkgPath + File.separator + "conf", pkgPath + File.separator + "libs"); } private Class lookupCatalogProvider(String provider, ClassLoader cl) { diff --git a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java index 901c3eccadf..d702743f55f 100644 --- a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java +++ b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java @@ -75,8 +75,8 @@ public Map fields() { } /** - * Merge the properties of the catalog with the given properties. - * Note: Properties in catalog entity has higher priority than the given properties. + * Merge the properties of the catalog with the given properties. Note: Properties in catalog + * entity has higher priority than the given properties. */ public void mergeProperty(Map extraProperty) { if (extraProperty == null) { From 69aa11bc2dbadc9f09e2db89087b1f270edacdf7 Mon Sep 17 00:00:00 2001 From: yuqi Date: Wed, 11 Oct 2023 23:09:32 +0800 Subject: [PATCH 07/23] Fix discussion one --- .../catalog/hive/HiveCatalogOperations.java | 12 +- .../src/main/resources/hive-site.xml_template | 16 +++ .../hive/TestHiveCatalogOperations.java | 36 ++++++ .../src/test/resources/hive-site.xml | 16 +++ .../graviton/catalog/BaseCatalog.java | 7 ++ .../graviton/catalog/CatalogManager.java | 106 +++++++++++------- .../graviton/catalog/TestCatalogManager.java | 18 +-- .../test/catalog/hive/CatalogHiveIT.java | 2 +- 8 files changed, 160 insertions(+), 53 deletions(-) create mode 100644 catalogs/catalog-hive/src/main/resources/hive-site.xml_template create mode 100644 catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java create mode 100644 catalogs/catalog-hive/src/test/resources/hive-site.xml diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index 2d32f07d8bd..d9769da33ac 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -4,6 +4,7 @@ */ package com.datastrato.graviton.catalog.hive; +import static com.datastrato.graviton.catalog.BaseCatalog.CATALOG_BYPASS_PREFIX; import static com.datastrato.graviton.catalog.hive.HiveTable.SUPPORT_TABLE_TYPES; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.COMMENT; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.TABLE_TYPE; @@ -63,7 +64,7 @@ public class HiveCatalogOperations implements CatalogOperations, SupportsSchemas @VisibleForTesting HiveClientPool clientPool; - private HiveConf hiveConf; + @VisibleForTesting HiveConf hiveConf; private final CatalogEntity entity; @@ -90,6 +91,15 @@ public void initialize(Map conf) throws RuntimeException { conf.forEach(hadoopConf::set); hiveConf = new HiveConf(hadoopConf, HiveCatalogOperations.class); + // Overwrite hive conf with graviton conf if exists + conf.forEach( + (key, value) -> { + if (key.startsWith(CATALOG_BYPASS_PREFIX)) { + // Trim bypass prefix + hiveConf.set(key.substring(CATALOG_BYPASS_PREFIX.length()), value); + } + }); + // todo(xun): add hive client pool size in config this.clientPool = new HiveClientPool(1, hiveConf); diff --git a/catalogs/catalog-hive/src/main/resources/hive-site.xml_template b/catalogs/catalog-hive/src/main/resources/hive-site.xml_template new file mode 100644 index 00000000000..97626980c0f --- /dev/null +++ b/catalogs/catalog-hive/src/main/resources/hive-site.xml_template @@ -0,0 +1,16 @@ + + + + hive.metastore.client.capability.check + true + + + + mapreduce.job.reduces + 10 + + + diff --git a/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java b/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java new file mode 100644 index 00000000000..14b2dff3632 --- /dev/null +++ b/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java @@ -0,0 +1,36 @@ +/* + * Copyright 2023 Datastrato. + * This software is licensed under the Apache License version 2. + */ + +package com.datastrato.graviton.catalog.hive; + +import static com.datastrato.graviton.catalog.BaseCatalog.CATALOG_BYPASS_PREFIX; + +import com.google.common.collect.Maps; +import java.util.Map; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class TestHiveCatalogOperations { + @Test + void testLoadConfig() { + Map properties = Maps.newHashMap(); + HiveCatalogOperations hiveCatalogOperations = new HiveCatalogOperations(null); + hiveCatalogOperations.initialize(properties); + String v = hiveCatalogOperations.hiveConf.get("mapreduce.job.reduces"); + Assertions.assertEquals("10", v); + + // Test If we can override the value in hive-site.xml + properties.put(CATALOG_BYPASS_PREFIX + "mapreduce.job.reduces", "20"); + hiveCatalogOperations.initialize(properties); + v = hiveCatalogOperations.hiveConf.get("mapreduce.job.reduces"); + Assertions.assertEquals("20", v); + + properties.clear(); + properties.put("mapreduce.job.reduces", "30"); + hiveCatalogOperations.initialize(properties); + v = hiveCatalogOperations.hiveConf.get("mapreduce.job.reduces"); + Assertions.assertEquals("30", v); + } +} diff --git a/catalogs/catalog-hive/src/test/resources/hive-site.xml b/catalogs/catalog-hive/src/test/resources/hive-site.xml new file mode 100644 index 00000000000..97626980c0f --- /dev/null +++ b/catalogs/catalog-hive/src/test/resources/hive-site.xml @@ -0,0 +1,16 @@ + + + + hive.metastore.client.capability.check + true + + + + mapreduce.job.reduces + 10 + + + diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index 36aee2f7294..9de86743525 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -41,6 +41,13 @@ public abstract class BaseCatalog private volatile CatalogOperations ops; private volatile Map properties; + + // Any graviton configuration that starts with this prefix will be trim and passed to the specific + // catalog implementation. For example, if the configuration is + // "graviton.bypass.hive.metastore.uris", + // then we will trim the prefix and pass "hive.metastore.uris" to the hive catalog implementation. + public static final String CATALOG_BYPASS_PREFIX = "graviton.bypass."; + /** * Creates a new instance of CatalogOperations. * diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 41874911ebc..346b43ac931 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -36,7 +36,7 @@ import com.github.benmanes.caffeine.cache.Scheduler; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.collect.ImmutableMap; +import com.google.common.base.Throwables; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -45,6 +45,7 @@ import java.io.Closeable; import java.io.File; import java.io.IOException; +import java.io.InputStream; import java.time.Instant; import java.util.Arrays; import java.util.Collections; @@ -52,10 +53,12 @@ import java.util.List; import java.util.Map; import java.util.Optional; +import java.util.Properties; import java.util.ServiceLoader; import java.util.concurrent.ScheduledThreadPoolExecutor; import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; +import org.apache.commons.io.FileUtils; import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -64,11 +67,6 @@ public class CatalogManager implements SupportsCatalogs, Closeable { private static final Logger LOG = LoggerFactory.getLogger(CatalogManager.class); - // Any graviton configuration that starts with this prefix will be trim and passed to the specific - // catalog implementation. For example, if the configuration is - // "graviton.bypass.hive.metastore.uris", - // then we will trim the prefix and pass "hive.metastore.uris" to the hive catalog implementation. - public static final String CATALOG_BYPASS_PREFIX = "graviton.bypass."; /** Wrapper class for a catalog instance and its class loader. */ public static class CatalogWrapper { @@ -246,6 +244,11 @@ public Catalog createCatalog( String comment, Map properties) throws NoSuchMetalakeException, CatalogAlreadyExistsException { + + // load catalog-related configuration from catalog-specific configuration file + Map catalogSpecificConfig = loadCatalogSpecificConfig(provider); + Map mergedConfig = mergeConf(properties, catalogSpecificConfig); + long uid = idGenerator.nextId(); StringIdentifier stringId = StringIdentifier.fromId(uid); CatalogEntity e = @@ -256,7 +259,7 @@ public Catalog createCatalog( .withType(type) .withProvider(provider) .withComment(comment) - .withProperties(StringIdentifier.addToProperties(stringId, properties)) + .withProperties(StringIdentifier.addToProperties(stringId, mergedConfig)) .withAuditInfo( new AuditInfo.Builder() .withCreator("graviton") /* TODO. Should change to real user */ @@ -265,9 +268,6 @@ public Catalog createCatalog( .build(); try { - // We need to create the catalog wrapper before we put the entity into the store, because we - // need to load the catalog-specific properties from the catalog-specific config file. - CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(e)); store.executeInTransaction( () -> { NameIdentifier metalakeIdent = NameIdentifier.of(ident.namespace().levels()); @@ -279,16 +279,15 @@ public Catalog createCatalog( store.put(e, false /* overwrite */); return null; }); + CatalogWrapper wrapper = catalogCache.get(ident, id -> createCatalogWrapper(e)); return wrapper.catalog; } catch (EntityAlreadyExistsException e1) { LOG.warn("Catalog {} already exists", ident, e1); throw new CatalogAlreadyExistsException("Catalog " + ident + " already exists"); } catch (IllegalArgumentException | NoSuchMetalakeException e2) { - catalogCache.invalidate(ident); throw e2; } catch (Exception e3) { LOG.error("Failed to create catalog {}", ident, e3); - catalogCache.invalidate(ident); throw new RuntimeException(e3); } } @@ -449,15 +448,12 @@ private CatalogWrapper loadCatalogInternal(NameIdentifier ident) throws NoSuchCa } private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { - Map mergedConf = - entity.getProperties() == null - ? ImmutableMap.of() - : ImmutableMap.copyOf(entity.getProperties()); + Map conf = entity.getProperties(); String provider = entity.getProvider(); IsolatedClassLoader classLoader; if (config.get(Configs.CATALOG_LOAD_ISOLATED)) { - List pkgPaths = buildPkgPaths(mergedConf, provider); + List pkgPaths = buildPkgPaths(conf, provider); classLoader = IsolatedClassLoader.buildClassLoader(pkgPaths); } else { // This will use the current class loader, it is mainly used for test. @@ -471,25 +467,15 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { // Load catalog specific properties CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); - Map catalogSpecificConfig = - classLoader.withClassLoader( - cl -> catalog.loadCatalogSpecificProperties(provider + ".properties"), - RuntimeException.class); - // Merge catalog-specific configurations to the entity properties, as the entity only contains - // properties that are set by user currently. - entity.mergeProperty(catalogSpecificConfig); - - Map totalMergedConf = - mergeConf(Maps.newHashMap(mergedConf), catalogSpecificConfig); - Map bypassConfig = getBypassConfig(totalMergedConf); + // Map bypassConfig = getBypassConfig(conf); // Initialize the catalog - catalog.withCatalogConf(bypassConfig).withCatalogEntity(entity); + catalog.withCatalogConf(conf).withCatalogEntity(entity); // Validate catalog properties and initialize the config classLoader.withClassLoader( cl -> { - Map configWithoutId = Maps.newHashMap(totalMergedConf); + Map configWithoutId = Maps.newHashMap(conf); configWithoutId.remove(ID_KEY); catalog.ops().catalogPropertiesMetadata().validatePropertyForCreate(configWithoutId); @@ -533,15 +519,57 @@ private BaseCatalog createCatalogInstance(IsolatedClassLoader classLoader, St return catalog; } - private Map getBypassConfig(Map totalMergedConf) { - Map bypassConfig = Maps.newHashMap(); - totalMergedConf.forEach( - (key, value) -> { - if (key.startsWith(CATALOG_BYPASS_PREFIX)) { - bypassConfig.put(key.substring(CATALOG_BYPASS_PREFIX.length()), value); - } - }); - return bypassConfig; + private Map loadCatalogSpecificConfig(String provider) { + if ("test".equals(provider)) { + return Maps.newHashMap(); + } + + String catalogSpecificConfigFile = provider + ".properties"; + Map catalogSpecificConfig = Maps.newHashMap(); + + String gravitonHome = System.getenv("GRAVITON_HOME"); + Preconditions.checkArgument(gravitonHome != null, "GRAVITON_HOME not set"); + boolean testEnv = System.getenv("GRAVITON_TEST") != null; + + String fullPath; + if (testEnv) { + fullPath = + String.join( + File.separator, + gravitonHome, + "catalogs", + "catalog-" + provider, + "build", + "resources", + "main", + catalogSpecificConfigFile); + } else { + fullPath = + String.join( + File.separator, + gravitonHome, + "catalogs", + provider, + "conf", + catalogSpecificConfigFile); + } + + try { + InputStream inputStream = FileUtils.openInputStream(new File(fullPath)); + Properties loadProperties = new Properties(); + loadProperties.load(inputStream); + loadProperties.forEach( + (key, value) -> catalogSpecificConfig.put(key.toString(), value.toString())); + } catch (Exception e) { + // If the catalog-specific configuration file is not found, it will not be loaded. + // Should we throw exception directly? + LOG.error( + "Failed to load catalog specific configurations, file name: '{}', Exception:\n{}", + catalogSpecificConfigFile, + Throwables.getStackTraceAsString(e)); + throw new RuntimeException(e); + } + return catalogSpecificConfig; } static Map mergeConf(Map properties, Map conf) { diff --git a/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java index ab123cce881..bb05a38d06c 100644 --- a/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java +++ b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java @@ -106,7 +106,7 @@ void testLoadTable() throws IOException { .put("key2", "value2") .put("key1", "value1") .put("hidden_key", "hidden_value") - .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") + .put("mock", "mock") .build(); Assertions.assertDoesNotThrow( () -> @@ -137,7 +137,7 @@ void testPropertyValidationInAlter() throws IOException { ImmutableMap.builder() .put("key2", "value2") .put("key1", "value1") - .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") + .put("mock", "mock") .build(); Assertions.assertDoesNotThrow( () -> @@ -152,7 +152,7 @@ void testPropertyValidationInAlter() throws IOException { .put("key1", "value1") .put("key3", "3") .put("key4", "value4") - .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") + .put("mock", "mock") .build(); Assertions.assertDoesNotThrow( () -> @@ -201,10 +201,7 @@ void testPropertyValidationInCreate() throws IOException { // key1 is required; Map props1 = - ImmutableMap.builder() - .put("key2", "value2") - .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") - .build(); + ImmutableMap.builder().put("key2", "value2").put("mock", "mock").build(); IllegalArgumentException e1 = Assertions.assertThrows( IllegalArgumentException.class, @@ -218,10 +215,7 @@ void testPropertyValidationInCreate() throws IOException { // key2 is required; Map props2 = - ImmutableMap.builder() - .put("key1", "value1") - .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") - .build(); + ImmutableMap.builder().put("key1", "value1").put("mock", "mock").build(); e1 = Assertions.assertThrows( IllegalArgumentException.class, @@ -238,7 +232,7 @@ void testPropertyValidationInCreate() throws IOException { .put("key1", "value1") .put("key2", "value2") .put("key3", "a12a1a") - .put(CatalogManager.CATALOG_BYPASS_PREFIX + "mock", "mock") + .put("mock", "mock") .build(); e1 = Assertions.assertThrows( diff --git a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java index 2a7418c28a1..067dc9ca68d 100644 --- a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java +++ b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java @@ -4,7 +4,7 @@ */ package com.datastrato.graviton.integration.test.catalog.hive; -import static com.datastrato.graviton.catalog.CatalogManager.CATALOG_BYPASS_PREFIX; +import static com.datastrato.graviton.catalog.BaseCatalog.CATALOG_BYPASS_PREFIX; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.COMMENT; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.EXTERNAL; import static com.datastrato.graviton.catalog.hive.HiveTablePropertiesMetadata.FORMAT; From ec700177368c2f98599a4aaf5731b74ac1f3e275 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 09:33:19 +0800 Subject: [PATCH 08/23] Fix discussion about file name and descriptions --- catalogs/catalog-hive/build.gradle.kts | 14 ++++++++++++++ .../src/main/resources/hive-site.xml_template | 9 --------- .../main/resources/{hive.properties => hive.conf} | 0 .../catalog-lakehouse-iceberg/build.gradle.kts | 14 ++++++++++++++ ...e-iceberg.properties => lakehouse-iceberg.conf} | 4 ++-- .../graviton/catalog/CatalogManager.java | 7 +++---- .../test/catalog/hive/CatalogHiveIT.java | 2 +- 7 files changed, 34 insertions(+), 16 deletions(-) rename catalogs/catalog-hive/src/main/resources/{hive.properties => hive.conf} (100%) rename catalogs/catalog-lakehouse-iceberg/src/main/resources/{lakehouse-iceberg.properties => lakehouse-iceberg.conf} (54%) diff --git a/catalogs/catalog-hive/build.gradle.kts b/catalogs/catalog-hive/build.gradle.kts index a02f4a4d0c8..086af2861c0 100644 --- a/catalogs/catalog-hive/build.gradle.kts +++ b/catalogs/catalog-hive/build.gradle.kts @@ -94,6 +94,20 @@ tasks { val copyCatalogConfig by registering(Copy::class) { from("src/main/resources") into("${rootDir}/distribution/package/catalogs/hive/conf") + + include("**/*.xml") + include("**/*.conf") + + include("**/*_template") + rename { original -> if (original.endsWith("_template")) { + original.replace("_template", "") + } else { + original + }} + + exclude { details -> + details.file.isDirectory() + } } val copyLibAndConfig by registering(Copy::class) { diff --git a/catalogs/catalog-hive/src/main/resources/hive-site.xml_template b/catalogs/catalog-hive/src/main/resources/hive-site.xml_template index 97626980c0f..5fd225e1011 100644 --- a/catalogs/catalog-hive/src/main/resources/hive-site.xml_template +++ b/catalogs/catalog-hive/src/main/resources/hive-site.xml_template @@ -3,14 +3,5 @@ ~ This software is licensed under the Apache License version 2. --> - - hive.metastore.client.capability.check - true - - - - mapreduce.job.reduces - 10 - diff --git a/catalogs/catalog-hive/src/main/resources/hive.properties b/catalogs/catalog-hive/src/main/resources/hive.conf similarity index 100% rename from catalogs/catalog-hive/src/main/resources/hive.properties rename to catalogs/catalog-hive/src/main/resources/hive.conf diff --git a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts index 237ed5cd865..5e3cde04c69 100644 --- a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts +++ b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts @@ -94,6 +94,20 @@ tasks { val copyCatalogConfig by registering(Copy::class) { from("src/main/resources") into("${rootDir}/distribution/package/catalogs/lakehouse-iceberg/conf") + + include("**/*.xml") + include("**/*.conf") + + include("**/*_template") + rename { original -> if (original.endsWith("_template")) { + original.replace("_template", "") + } else { + original + }} + + exclude { details -> + details.file.isDirectory() + } } val copyLibAndConfig by registering(Copy::class) { diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf similarity index 54% rename from catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties rename to catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf index 2ea6df5dbd7..884cbdb2c07 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.properties +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf @@ -3,6 +3,6 @@ # This software is licensed under the Apache License version 2. # -## This file holds common properties for Iceberg catalog. The format of the key is +## This file holds common configurations for Lakehouse-iceberg catalog. The format of the key is ## 'graviton.bypass.{iceberg-inner-config-key}' and `iceberg-inner-config-key` is the -## key in the iceberg catalog configuration file. +## real key in the pass to Lakehouse-iceberg catalog. diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 346b43ac931..a9900cbf88b 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -36,7 +36,6 @@ import com.github.benmanes.caffeine.cache.Scheduler; import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; -import com.google.common.base.Throwables; import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -524,7 +523,7 @@ private Map loadCatalogSpecificConfig(String provider) { return Maps.newHashMap(); } - String catalogSpecificConfigFile = provider + ".properties"; + String catalogSpecificConfigFile = provider + ".conf"; Map catalogSpecificConfig = Maps.newHashMap(); String gravitonHome = System.getenv("GRAVITON_HOME"); @@ -564,9 +563,9 @@ private Map loadCatalogSpecificConfig(String provider) { // If the catalog-specific configuration file is not found, it will not be loaded. // Should we throw exception directly? LOG.error( - "Failed to load catalog specific configurations, file name: '{}', Exception:\n{}", + "Failed to load catalog specific configurations, file name: '{}'", catalogSpecificConfigFile, - Throwables.getStackTraceAsString(e)); + e); throw new RuntimeException(e); } return catalogSpecificConfig; diff --git a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java index 067dc9ca68d..3c04057e0d5 100644 --- a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java +++ b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java @@ -148,7 +148,7 @@ private static void createCatalog() { Catalog loadCatalog = metalake.loadCatalog(NameIdentifier.of(metalakeName, catalogName)); // We have added the bypass property 'graviton.bypass.hive.metastore.client.capability.check' - // to the hive.properties, so we need to check if the property has been added successfully. + // to the hive.conf, so we need to check if the property has been added successfully. Assertions.assertTrue( loadCatalog .properties() From 78941ca422522984de6244f788fae09556370521 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 09:37:31 +0800 Subject: [PATCH 09/23] Fix comments --- .../graviton/catalog/hive/HiveCatalogOperations.java | 2 +- .../src/main/resources/lakehouse-iceberg.conf | 2 +- .../java/com/datastrato/graviton/catalog/CatalogManager.java | 5 +---- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java index d9769da33ac..1c8bab6d56e 100644 --- a/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/main/java/com/datastrato/graviton/catalog/hive/HiveCatalogOperations.java @@ -95,7 +95,7 @@ public void initialize(Map conf) throws RuntimeException { conf.forEach( (key, value) -> { if (key.startsWith(CATALOG_BYPASS_PREFIX)) { - // Trim bypass prefix + // Trim bypass prefix and pass it to hive conf hiveConf.set(key.substring(CATALOG_BYPASS_PREFIX.length()), value); } }); diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf index 884cbdb2c07..66ace4ca340 100644 --- a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf @@ -5,4 +5,4 @@ ## This file holds common configurations for Lakehouse-iceberg catalog. The format of the key is ## 'graviton.bypass.{iceberg-inner-config-key}' and `iceberg-inner-config-key` is the -## real key in the pass to Lakehouse-iceberg catalog. +## real key that pass to Lakehouse-iceberg catalog. diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index a9900cbf88b..0f162bef5ad 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -463,13 +463,10 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { // Load Catalog class instance BaseCatalog catalog = createCatalogInstance(classLoader, provider); + catalog.withCatalogConf(conf).withCatalogEntity(entity); - // Load catalog specific properties CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); - - // Map bypassConfig = getBypassConfig(conf); // Initialize the catalog - catalog.withCatalogConf(conf).withCatalogEntity(entity); // Validate catalog properties and initialize the config classLoader.withClassLoader( From 9d24e45e24917d2c9d07d06e52e510870221f7c8 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 09:52:47 +0800 Subject: [PATCH 10/23] Remove used code --- .../hive/TestHiveCatalogOperations.java | 5 +++-- .../graviton/catalog/BaseCatalog.java | 22 ------------------- core/src/test/resources/test.properties | 4 ---- .../graviton/server/web/rest/TestCatalog.java | 11 ---------- 4 files changed, 3 insertions(+), 39 deletions(-) delete mode 100644 core/src/test/resources/test.properties diff --git a/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java b/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java index 14b2dff3632..51bf05c9f99 100644 --- a/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java +++ b/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java @@ -12,9 +12,9 @@ import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; -public class TestHiveCatalogOperations { +class TestHiveCatalogOperations { @Test - void testLoadConfig() { + void testInitialize() { Map properties = Maps.newHashMap(); HiveCatalogOperations hiveCatalogOperations = new HiveCatalogOperations(null); hiveCatalogOperations.initialize(properties); @@ -27,6 +27,7 @@ void testLoadConfig() { v = hiveCatalogOperations.hiveConf.get("mapreduce.job.reduces"); Assertions.assertEquals("20", v); + // Test If user properties can override the value in hive-site.xml properties.clear(); properties.put("mapreduce.job.reduces", "30"); hiveCatalogOperations.initialize(properties); diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index 9de86743525..3dcc1a2174f 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -9,11 +9,8 @@ import com.datastrato.graviton.CatalogProvider; import com.datastrato.graviton.meta.CatalogEntity; import com.google.common.base.Preconditions; -import com.google.common.base.Throwables; import com.google.common.collect.Maps; -import java.io.InputStream; import java.util.Map; -import java.util.Properties; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -56,25 +53,6 @@ public abstract class BaseCatalog */ protected abstract CatalogOperations newOps(Map config); - /** Load properties for the catalog from file in the classpath. */ - public Map loadCatalogSpecificProperties(String fileURL) { - Map configMap = Maps.newHashMap(); - try { - InputStream inputStream = this.getClass().getClassLoader().getResourceAsStream(fileURL); - Properties loadProperties = new Properties(); - loadProperties.load(inputStream); - loadProperties.forEach((key, value) -> configMap.put(key.toString(), value.toString())); - } catch (Exception e) { - // If the catalog-specific configuration file is not found, it will not be loaded. - // Should we throw exception directly? - LOG.warn( - "Failed to load catalog specific configurations, file name: '{}', Exception:\n{}", - fileURL, - Throwables.getStackTraceAsString(e)); - } - return configMap; - } - @Override public PropertiesMetadata tablePropertiesMetadata() throws UnsupportedOperationException { return ops().tablePropertiesMetadata(); diff --git a/core/src/test/resources/test.properties b/core/src/test/resources/test.properties deleted file mode 100644 index 0f778e5b640..00000000000 --- a/core/src/test/resources/test.properties +++ /dev/null @@ -1,4 +0,0 @@ -# -# Copyright 2023 Datastrato. -# This software is licensed under the Apache License version 2. -# \ No newline at end of file diff --git a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java index 09b43e4fe13..b6825f67252 100644 --- a/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java +++ b/server/src/test/java/com/datastrato/graviton/server/web/rest/TestCatalog.java @@ -10,8 +10,6 @@ import com.google.common.collect.Maps; import java.io.IOException; import java.util.Map; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.Test; public class TestCatalog extends BaseCatalog { @Override @@ -38,13 +36,4 @@ public PropertiesMetadata catalogPropertiesMetadata() throws UnsupportedOperatio } }; } - - @Test - void test() { - TestCatalog testCatalog = new TestCatalog(); - // This will load the log4j2.properties file from the classpath - Map configs = testCatalog.loadCatalogSpecificProperties("log4j2.properties"); - Assertions.assertTrue(configs.containsKey("status")); - Assertions.assertEquals("warn", configs.get("status")); - } } From 1f6c2354bb9f1da43e3a3d6900e2cd5e127d037f Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 09:55:30 +0800 Subject: [PATCH 11/23] Remove used code again --- .../graviton/meta/CatalogEntity.java | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java index d702743f55f..f257b2262a2 100644 --- a/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java +++ b/core/src/main/java/com/datastrato/graviton/meta/CatalogEntity.java @@ -12,8 +12,6 @@ import com.datastrato.graviton.HasIdentifier; import com.datastrato.graviton.Namespace; import com.google.common.base.Objects; -import com.google.common.collect.ImmutableMap; -import com.google.common.collect.Maps; import java.util.Collections; import java.util.HashMap; import java.util.Map; @@ -74,23 +72,6 @@ public Map fields() { return Collections.unmodifiableMap(fields); } - /** - * Merge the properties of the catalog with the given properties. Note: Properties in catalog - * entity has higher priority than the given properties. - */ - public void mergeProperty(Map extraProperty) { - if (extraProperty == null) { - return; - } - - if (properties == null) { - properties = Maps.newHashMap(extraProperty); - } else { - extraProperty.putAll(properties); - properties = ImmutableMap.copyOf(extraProperty); - } - } - /** * The audit information of the catalog. * From 97d31e872e7a78852768732d7456879ddb4f5798 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 10:11:47 +0800 Subject: [PATCH 12/23] Change the format of the name for template file --- catalogs/catalog-hive/build.gradle.kts | 6 +++--- .../{hive-site.xml_template => hive-site.xml.template} | 0 catalogs/catalog-lakehouse-iceberg/build.gradle.kts | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) rename catalogs/catalog-hive/src/main/resources/{hive-site.xml_template => hive-site.xml.template} (100%) diff --git a/catalogs/catalog-hive/build.gradle.kts b/catalogs/catalog-hive/build.gradle.kts index 086af2861c0..c3b254ee249 100644 --- a/catalogs/catalog-hive/build.gradle.kts +++ b/catalogs/catalog-hive/build.gradle.kts @@ -98,9 +98,9 @@ tasks { include("**/*.xml") include("**/*.conf") - include("**/*_template") - rename { original -> if (original.endsWith("_template")) { - original.replace("_template", "") + include("**/*.template") + rename { original -> if (original.endsWith(".template")) { + original.replace(".template", "") } else { original }} diff --git a/catalogs/catalog-hive/src/main/resources/hive-site.xml_template b/catalogs/catalog-hive/src/main/resources/hive-site.xml.template similarity index 100% rename from catalogs/catalog-hive/src/main/resources/hive-site.xml_template rename to catalogs/catalog-hive/src/main/resources/hive-site.xml.template diff --git a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts index 5e3cde04c69..7d50a5fa266 100644 --- a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts +++ b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts @@ -98,9 +98,9 @@ tasks { include("**/*.xml") include("**/*.conf") - include("**/*_template") - rename { original -> if (original.endsWith("_template")) { - original.replace("_template", "") + include("**/*.template") + rename { original -> if (original.endsWith(".template")) { + original.replace(".template", "") } else { original }} From 249334b0d8910f01a25a6de70376c124a2aa42ec Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 11:15:48 +0800 Subject: [PATCH 13/23] Only configuration files with specific names will be copied to classpath --- catalogs/catalog-hive/build.gradle.kts | 4 ++-- catalogs/catalog-lakehouse-iceberg/build.gradle.kts | 4 +--- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/catalogs/catalog-hive/build.gradle.kts b/catalogs/catalog-hive/build.gradle.kts index c3b254ee249..d9eb75091bd 100644 --- a/catalogs/catalog-hive/build.gradle.kts +++ b/catalogs/catalog-hive/build.gradle.kts @@ -95,8 +95,8 @@ tasks { from("src/main/resources") into("${rootDir}/distribution/package/catalogs/hive/conf") - include("**/*.xml") - include("**/*.conf") + include("hive.conf") + include("hive-site.xml.template") include("**/*.template") rename { original -> if (original.endsWith(".template")) { diff --git a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts index 7d50a5fa266..9065258fe18 100644 --- a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts +++ b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts @@ -95,10 +95,8 @@ tasks { from("src/main/resources") into("${rootDir}/distribution/package/catalogs/lakehouse-iceberg/conf") - include("**/*.xml") - include("**/*.conf") + include("lakehouse-iceberg.conf") - include("**/*.template") rename { original -> if (original.endsWith(".template")) { original.replace(".template", "") } else { From b0432dd6bf8df866be4647d87b0e19b33af5b6bd Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 11:51:32 +0800 Subject: [PATCH 14/23] Changes names and some minor logic according to review comments --- .../graviton/catalog/BaseCatalog.java | 2 +- .../graviton/catalog/CatalogManager.java | 18 ++++------ .../graviton/utils/IsolatedClassLoader.java | 33 +++++++++---------- 3 files changed, 24 insertions(+), 29 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index 3dcc1a2174f..b50a626897d 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -39,7 +39,7 @@ public abstract class BaseCatalog private volatile Map properties; - // Any graviton configuration that starts with this prefix will be trim and passed to the specific + // Any Graviton configuration that starts with this prefix will be trim and passed to the specific // catalog implementation. For example, if the configuration is // "graviton.bypass.hive.metastore.uris", // then we will trim the prefix and pass "hive.metastore.uris" to the hive catalog implementation. diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 0f162bef5ad..84ddfa28a15 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -452,8 +452,8 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { IsolatedClassLoader classLoader; if (config.get(Configs.CATALOG_LOAD_ISOLATED)) { - List pkgPaths = buildPkgPaths(conf, provider); - classLoader = IsolatedClassLoader.buildClassLoader(pkgPaths); + List libAndResourcePaths = buildLibAndResourcePaths(conf, provider); + classLoader = IsolatedClassLoader.buildClassLoader(libAndResourcePaths); } else { // This will use the current class loader, it is mainly used for test. classLoader = @@ -466,8 +466,6 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { catalog.withCatalogConf(conf).withCatalogEntity(entity); CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); - // Initialize the catalog - // Validate catalog properties and initialize the config classLoader.withClassLoader( cl -> { @@ -478,9 +476,8 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { // Call wrapper.catalog.properties() to make BaseCatalog#properties in IsolatedClassLoader // not null. Why we do this? Because wrapper.catalog.properties() need to be called in the // IsolatedClassLoader, it needs to load the specific catalog class such as HiveCatalog or - // so. - // For simply, We will preload the value of properties and thus AppClassLoader can get the - // value of properties. + // so. For simply, We will preload the value of properties and thus AppClassLoader can get + // the value of properties. wrapper.catalog.properties(); return null; }, @@ -550,8 +547,7 @@ private Map loadCatalogSpecificConfig(String provider) { catalogSpecificConfigFile); } - try { - InputStream inputStream = FileUtils.openInputStream(new File(fullPath)); + try (InputStream inputStream = FileUtils.openInputStream(new File(fullPath))) { Properties loadProperties = new Properties(); loadProperties.load(inputStream); loadProperties.forEach( @@ -584,7 +580,7 @@ private Map catalogConf(String name, Config config) { return config.getConfigsWithPrefix(confPrefix); } - private List buildPkgPaths(Map conf, String provider) { + private List buildLibAndResourcePaths(Map conf, String provider) { String pkg = conf.get(Catalog.PROPERTY_PACKAGE); String gravitonHome = System.getenv("GRAVITON_HOME"); @@ -593,7 +589,7 @@ private List buildPkgPaths(Map conf, String provider) { String pkgPath; if (pkg != null) { - return Lists.newArrayList(pkg.split(",")); + return Lists.newArrayList(pkg); } else if (testEnv) { // In test, the catalog package is under the build directory. pkgPath = diff --git a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java index fc1e2ddf1d9..9263b776687 100644 --- a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java +++ b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java @@ -88,43 +88,42 @@ public T withClassLoader( } } - public static IsolatedClassLoader buildClassLoader(List pkgPaths) { - // Listing all the jars under the package path and build the isolated class loader. - - List jars = Lists.newArrayList(); - for (String path : pkgPaths) { - File pkgFolder = new File(path); - if (!pkgFolder.exists() - || !pkgFolder.isDirectory() - || !pkgFolder.canRead() - || !pkgFolder.canExecute()) { - throw new IllegalArgumentException("Invalid package path: " + pkgPaths); + public static IsolatedClassLoader buildClassLoader(List libAndResourcesPaths) { + // Listing all the classPath under the package path and build the isolated class loader. + List classPathContents = Lists.newArrayList(); + for (String path : libAndResourcesPaths) { + File folder = new File(path); + if (!folder.exists() || !folder.isDirectory() || !folder.canRead() || !folder.canExecute()) { + throw new IllegalArgumentException( + String.format("Invalid package path: %s in %s", path, libAndResourcesPaths)); } - // Add all the jars under the package path. - Arrays.stream(pkgFolder.listFiles()) + // Add all the jar under the folder to classpath. + Arrays.stream(folder.listFiles()) + .filter(f -> f.getName().endsWith(".jar")) .forEach( f -> { try { - jars.add(f.toURI().toURL()); + classPathContents.add(f.toURI().toURL()); } catch (MalformedURLException e) { LOG.warn("Failed to read jar file: {}", f.getAbsolutePath(), e); } }); // Add itself to the classpath. - Lists.newArrayList(pkgFolder).stream() + Lists.newArrayList(folder).stream() .forEach( f -> { try { - jars.add(f.toURI().toURL()); + classPathContents.add(f.toURI().toURL()); } catch (MalformedURLException e) { LOG.warn("Failed to read jar file: {}", f.getAbsolutePath(), e); } }); } - return new IsolatedClassLoader(jars, Collections.emptyList(), Collections.emptyList()); + return new IsolatedClassLoader( + classPathContents, Collections.emptyList(), Collections.emptyList()); } /** Closes the class loader. */ From c1b6e3389ce30450d3ccb5080051601ecc38dfb7 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 11:53:53 +0800 Subject: [PATCH 15/23] Remove some incorrect comments --- .../java/com/datastrato/graviton/catalog/CatalogManager.java | 2 -- 1 file changed, 2 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 84ddfa28a15..3ceeb0cf954 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -553,8 +553,6 @@ private Map loadCatalogSpecificConfig(String provider) { loadProperties.forEach( (key, value) -> catalogSpecificConfig.put(key.toString(), value.toString())); } catch (Exception e) { - // If the catalog-specific configuration file is not found, it will not be loaded. - // Should we throw exception directly? LOG.error( "Failed to load catalog specific configurations, file name: '{}'", catalogSpecificConfigFile, From 6973eaf2d7bc75a9c28bc960383844750a6154ae Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 12:02:45 +0800 Subject: [PATCH 16/23] Changes name of some variants in `buildLibAndResourcePaths` --- .../graviton/catalog/CatalogManager.java | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 3ceeb0cf954..82e7aea9f98 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -579,27 +579,28 @@ private Map catalogConf(String name, Config config) { } private List buildLibAndResourcePaths(Map conf, String provider) { - String pkg = conf.get(Catalog.PROPERTY_PACKAGE); - String gravitonHome = System.getenv("GRAVITON_HOME"); Preconditions.checkArgument(gravitonHome != null, "GRAVITON_HOME not set"); boolean testEnv = System.getenv("GRAVITON_TEST") != null; - String pkgPath; + String pkg = conf.get(Catalog.PROPERTY_PACKAGE); if (pkg != null) { + // Only libs will be added to the classpath. return Lists.newArrayList(pkg); } else if (testEnv) { // In test, the catalog package is under the build directory. - pkgPath = + String buildDirForTest = String.join(File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build"); // Add the config and lib to the classpath. return Lists.newArrayList( - pkgPath + File.separator + "resources", pkgPath + File.separator + "libs"); + buildDirForTest + File.separator + "resources", + buildDirForTest + File.separator + "libs"); } // In real environment, the catalog package is under the catalog directory. - pkgPath = String.join(File.separator, gravitonHome, "catalogs", provider); - return Lists.newArrayList(pkgPath + File.separator + "conf", pkgPath + File.separator + "libs"); + String classPathDir = String.join(File.separator, gravitonHome, "catalogs", provider); + return Lists.newArrayList( + classPathDir + File.separator + "conf", classPathDir + File.separator + "libs"); } private Class lookupCatalogProvider(String provider, ClassLoader cl) { From 30ab0eaa0ada415317b3fcf8708994bbb6b09368 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 13:58:50 +0800 Subject: [PATCH 17/23] Remove unnecessary code --- catalogs/catalog-hive/build.gradle.kts | 1 - 1 file changed, 1 deletion(-) diff --git a/catalogs/catalog-hive/build.gradle.kts b/catalogs/catalog-hive/build.gradle.kts index d9eb75091bd..d458f3d27a4 100644 --- a/catalogs/catalog-hive/build.gradle.kts +++ b/catalogs/catalog-hive/build.gradle.kts @@ -98,7 +98,6 @@ tasks { include("hive.conf") include("hive-site.xml.template") - include("**/*.template") rename { original -> if (original.endsWith(".template")) { original.replace(".template", "") } else { From 299c33c72c5b8f5ecd0840e91f7b4cb483d9edb1 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 15:54:59 +0800 Subject: [PATCH 18/23] Separate config and package --- .../EntityAlreadyExistsException.java | 15 +++++ .../graviton/catalog/CatalogManager.java | 56 +++++++++++-------- 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java b/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java index 21fa114655d..58bf4d84bb1 100644 --- a/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java +++ b/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java @@ -4,6 +4,11 @@ */ package com.datastrato.graviton; +import com.google.common.collect.Lists; +import com.google.common.collect.Maps; +import java.util.List; +import java.util.Map; + /** * Exception class indicating that an entity already exists. This exception is thrown when an * attempt is made to create an entity that already exists within the Graviton framework. @@ -28,4 +33,14 @@ public EntityAlreadyExistsException(String message) { public EntityAlreadyExistsException(String message, Throwable cause) { super(message, cause); } + + public static void main(String[] args) { + List list = Lists.newArrayList(); + list.add("a"); + System.out.println(list); + + Map maps = Maps.newHashMap(); + maps.put("a", "b"); + System.out.println(maps); + } } diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 82e7aea9f98..ef6db70b1d3 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -58,6 +58,7 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.apache.commons.io.FileUtils; +import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -452,8 +453,9 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { IsolatedClassLoader classLoader; if (config.get(Configs.CATALOG_LOAD_ISOLATED)) { - List libAndResourcePaths = buildLibAndResourcePaths(conf, provider); - classLoader = IsolatedClassLoader.buildClassLoader(libAndResourcePaths); + String pkgPath = buildPkgPath(conf, provider); + String confPath = buildConfPath(pkgPath); + classLoader = IsolatedClassLoader.buildClassLoader(Lists.newArrayList(pkgPath, confPath)); } else { // This will use the current class loader, it is mainly used for test. classLoader = @@ -553,11 +555,10 @@ private Map loadCatalogSpecificConfig(String provider) { loadProperties.forEach( (key, value) -> catalogSpecificConfig.put(key.toString(), value.toString())); } catch (Exception e) { - LOG.error( + LOG.warn( "Failed to load catalog specific configurations, file name: '{}'", catalogSpecificConfigFile, e); - throw new RuntimeException(e); } return catalogSpecificConfig; } @@ -568,39 +569,48 @@ static Map mergeConf(Map properties, Map catalogConf(String name, Config config) { - String confPrefix = "graviton.catalog." + name + "."; - return config.getConfigsWithPrefix(confPrefix); + private String buildConfPath(String pkgPath) { + boolean testEnv = System.getenv("GRAVITON_TEST") != null; + if (testEnv) { + String[] pkgPathParts = pkgPath.split(File.separator); + pkgPathParts[pkgPathParts.length - 1] = "resources"; + ArrayUtils.add(pkgPathParts, "main"); + return String.join(File.separator, pkgPathParts); + } + + return deduceConfPath(pkgPath); } - private List buildLibAndResourcePaths(Map conf, String provider) { + private String buildPkgPath(Map conf, String provider) { String gravitonHome = System.getenv("GRAVITON_HOME"); Preconditions.checkArgument(gravitonHome != null, "GRAVITON_HOME not set"); boolean testEnv = System.getenv("GRAVITON_TEST") != null; String pkg = conf.get(Catalog.PROPERTY_PACKAGE); + String pkgPath; if (pkg != null) { - // Only libs will be added to the classpath. - return Lists.newArrayList(pkg); + pkgPath = pkg; } else if (testEnv) { // In test, the catalog package is under the build directory. - String buildDirForTest = - String.join(File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build"); - // Add the config and lib to the classpath. - return Lists.newArrayList( - buildDirForTest + File.separator + "resources", - buildDirForTest + File.separator + "libs"); + pkgPath = + String.join( + File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build", "libs"); + } else { + // In real environment, the catalog package is under the catalog directory. + pkgPath = String.join(File.separator, gravitonHome, "catalogs", provider, "libs"); } - // In real environment, the catalog package is under the catalog directory. - String classPathDir = String.join(File.separator, gravitonHome, "catalogs", provider); - return Lists.newArrayList( - classPathDir + File.separator + "conf", classPathDir + File.separator + "libs"); + return pkgPath; } private Class lookupCatalogProvider(String provider, ClassLoader cl) { From 2677a7c914cba73e74d4ec88dcf51990fd3b6b18 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 16:26:58 +0800 Subject: [PATCH 19/23] Remove unused code --- .../graviton/EntityAlreadyExistsException.java | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java b/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java index 58bf4d84bb1..74599bd6c13 100644 --- a/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java +++ b/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java @@ -33,14 +33,4 @@ public EntityAlreadyExistsException(String message) { public EntityAlreadyExistsException(String message, Throwable cause) { super(message, cause); } - - public static void main(String[] args) { - List list = Lists.newArrayList(); - list.add("a"); - System.out.println(list); - - Map maps = Maps.newHashMap(); - maps.put("a", "b"); - System.out.println(maps); - } } From 2473b24457b6612f8112e29eb9ca365454efb488 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 16:43:31 +0800 Subject: [PATCH 20/23] Remove unused code --- .../datastrato/graviton/EntityAlreadyExistsException.java | 5 ----- 1 file changed, 5 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java b/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java index 74599bd6c13..21fa114655d 100644 --- a/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java +++ b/core/src/main/java/com/datastrato/graviton/EntityAlreadyExistsException.java @@ -4,11 +4,6 @@ */ package com.datastrato.graviton; -import com.google.common.collect.Lists; -import com.google.common.collect.Maps; -import java.util.List; -import java.util.Map; - /** * Exception class indicating that an entity already exists. This exception is thrown when an * attempt is made to create an entity that already exists within the Graviton framework. From 1138d3f7d55c01123667e0aa309457bf675ce389 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 18:03:18 +0800 Subject: [PATCH 21/23] Refactor the logic of getting config file for specific catalog --- .../graviton/catalog/CatalogManager.java | 31 +++++++++---------- 1 file changed, 15 insertions(+), 16 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index ef6db70b1d3..8f0f47431ae 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -58,7 +58,6 @@ import java.util.concurrent.TimeUnit; import java.util.stream.Collectors; import org.apache.commons.io.FileUtils; -import org.apache.commons.lang3.ArrayUtils; import org.apache.commons.lang3.tuple.Pair; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -454,7 +453,7 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { IsolatedClassLoader classLoader; if (config.get(Configs.CATALOG_LOAD_ISOLATED)) { String pkgPath = buildPkgPath(conf, provider); - String confPath = buildConfPath(pkgPath); + String confPath = buildConfPath(provider); classLoader = IsolatedClassLoader.buildClassLoader(Lists.newArrayList(pkgPath, confPath)); } else { // This will use the current class loader, it is mainly used for test. @@ -569,26 +568,26 @@ static Map mergeConf(Map properties, Map conf, String provider) { From 8474e5f7b4d3c22c5b61b286a2561794bf374406 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 20:18:09 +0800 Subject: [PATCH 22/23] Fix comments by xiaojin --- .../build.gradle.kts | 2 ++ .../src/main/resources/hdfs-site.xml.template | 6 ++++ .../src/main/resources/hive-site.xml.template | 7 +++++ .../graviton/catalog/BaseCatalog.java | 2 +- .../graviton/catalog/CatalogManager.java | 28 +------------------ .../graviton/utils/IsolatedClassLoader.java | 2 +- .../test/catalog/hive/CatalogHiveIT.java | 11 -------- 7 files changed, 18 insertions(+), 40 deletions(-) create mode 100644 catalogs/catalog-lakehouse-iceberg/src/main/resources/hdfs-site.xml.template create mode 100644 catalogs/catalog-lakehouse-iceberg/src/main/resources/hive-site.xml.template diff --git a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts index 9065258fe18..b8ed8c6b4be 100644 --- a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts +++ b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts @@ -96,6 +96,8 @@ tasks { into("${rootDir}/distribution/package/catalogs/lakehouse-iceberg/conf") include("lakehouse-iceberg.conf") + include("hive-site.xml.template") + include("hdfs-site.xml.template") rename { original -> if (original.endsWith(".template")) { original.replace(".template", "") diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/resources/hdfs-site.xml.template b/catalogs/catalog-lakehouse-iceberg/src/main/resources/hdfs-site.xml.template new file mode 100644 index 00000000000..231c7c49105 --- /dev/null +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/hdfs-site.xml.template @@ -0,0 +1,6 @@ + + + \ No newline at end of file diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/resources/hive-site.xml.template b/catalogs/catalog-lakehouse-iceberg/src/main/resources/hive-site.xml.template new file mode 100644 index 00000000000..5fd225e1011 --- /dev/null +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/hive-site.xml.template @@ -0,0 +1,7 @@ + + + + diff --git a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java index b50a626897d..03092a74a80 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -42,7 +42,7 @@ public abstract class BaseCatalog // Any Graviton configuration that starts with this prefix will be trim and passed to the specific // catalog implementation. For example, if the configuration is // "graviton.bypass.hive.metastore.uris", - // then we will trim the prefix and pass "hive.metastore.uris" to the hive catalog implementation. + // then we will trim the prefix and pass "hive.metastore.uris" to the hive client configurations. public static final String CATALOG_BYPASS_PREFIX = "graviton.bypass."; /** diff --git a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java index 8f0f47431ae..002f318a782 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/CatalogManager.java @@ -521,33 +521,7 @@ private Map loadCatalogSpecificConfig(String provider) { String catalogSpecificConfigFile = provider + ".conf"; Map catalogSpecificConfig = Maps.newHashMap(); - String gravitonHome = System.getenv("GRAVITON_HOME"); - Preconditions.checkArgument(gravitonHome != null, "GRAVITON_HOME not set"); - boolean testEnv = System.getenv("GRAVITON_TEST") != null; - - String fullPath; - if (testEnv) { - fullPath = - String.join( - File.separator, - gravitonHome, - "catalogs", - "catalog-" + provider, - "build", - "resources", - "main", - catalogSpecificConfigFile); - } else { - fullPath = - String.join( - File.separator, - gravitonHome, - "catalogs", - provider, - "conf", - catalogSpecificConfigFile); - } - + String fullPath = buildConfPath(provider) + File.separator + catalogSpecificConfigFile; try (InputStream inputStream = FileUtils.openInputStream(new File(fullPath))) { Properties loadProperties = new Properties(); loadProperties.load(inputStream); diff --git a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java index 9263b776687..280e97cb44b 100644 --- a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java +++ b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java @@ -117,7 +117,7 @@ public static IsolatedClassLoader buildClassLoader(List libAndResourcesP try { classPathContents.add(f.toURI().toURL()); } catch (MalformedURLException e) { - LOG.warn("Failed to read jar file: {}", f.getAbsolutePath(), e); + LOG.warn("Failed to read directory: {}", f.getAbsolutePath(), e); } }); } diff --git a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java index 3c04057e0d5..6a59f3a9aff 100644 --- a/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java +++ b/integration-test/src/test/java/com/datastrato/graviton/integration/test/catalog/hive/CatalogHiveIT.java @@ -147,17 +147,6 @@ private static void createCatalog() { properties); Catalog loadCatalog = metalake.loadCatalog(NameIdentifier.of(metalakeName, catalogName)); - // We have added the bypass property 'graviton.bypass.hive.metastore.client.capability.check' - // to the hive.conf, so we need to check if the property has been added successfully. - Assertions.assertTrue( - loadCatalog - .properties() - .containsKey("graviton.bypass.hive.metastore.client.capability.check")); - Assertions.assertEquals( - "false", - loadCatalog.properties().get("graviton.bypass.hive.metastore.client.capability.check")); - Assertions.assertEquals(createdCatalog, loadCatalog); - catalog = loadCatalog; } From 7b42c7db08b41ce98d690ef701846908bbc9b5b0 Mon Sep 17 00:00:00 2001 From: yuqi Date: Thu, 12 Oct 2023 20:32:16 +0800 Subject: [PATCH 23/23] refine code --- .../graviton/utils/IsolatedClassLoader.java | 14 +++++--------- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java index 280e97cb44b..f61907e6430 100644 --- a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java +++ b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java @@ -111,15 +111,11 @@ public static IsolatedClassLoader buildClassLoader(List libAndResourcesP }); // Add itself to the classpath. - Lists.newArrayList(folder).stream() - .forEach( - f -> { - try { - classPathContents.add(f.toURI().toURL()); - } catch (MalformedURLException e) { - LOG.warn("Failed to read directory: {}", f.getAbsolutePath(), e); - } - }); + try { + classPathContents.add(folder.toURI().toURL()); + } catch (MalformedURLException e) { + LOG.warn("Failed to read directory: {}", folder.getAbsolutePath(), e); + } } return new IsolatedClassLoader(