From 4d9e7a955377763697f86b381c02252f7d070403 Mon Sep 17 00:00:00 2001 From: Qi Yu Date: Thu, 12 Oct 2023 20:57:14 +0800 Subject: [PATCH] [#478] feat(core, catalogs): Refactor catalog configurations (#489) ### What changes were proposed in this pull request? In this PR, we introduce the following points: - Remove the catalog-related configuration from `Graviton` server configuration file. - Support setting catalog configuration with a separate config file located in `catalogs` module - Provide catalog-related configurations for a specific engine with an unique prefix. ### Why are the changes needed? Fix: #478 Fix: #196 ### Does this PR introduce _any_ user-facing change? N/A ### How was this patch tested? New UTs --- build.gradle.kts | 8 +- catalogs/catalog-hive/build.gradle.kts | 22 ++ .../catalog/hive/HiveCatalogOperations.java | 12 +- .../src/main/resources/hive-site.xml.template | 7 + .../catalog-hive/src/main/resources/hive.conf | 12 ++ .../hive/TestHiveCatalogOperations.java | 37 ++++ .../src/test/resources/hive-site.xml | 16 ++ .../build.gradle.kts | 23 ++ .../src/main/resources/hdfs-site.xml.template | 6 + .../src/main/resources/hive-site.xml.template | 7 + .../src/main/resources/lakehouse-iceberg.conf | 8 + .../graviton/aux/AuxiliaryServiceManager.java | 7 +- .../graviton/catalog/BaseCatalog.java | 11 + .../graviton/catalog/CatalogManager.java | 199 ++++++++++-------- .../graviton/utils/IsolatedClassLoader.java | 64 ++++-- .../aux/TestAuxiliaryServiceManager.java | 4 +- .../graviton/catalog/TestCatalogManager.java | 3 + .../test/catalog/hive/CatalogHiveIT.java | 15 +- .../{java => }/resources/log4j2.properties | 0 19 files changed, 343 insertions(+), 118 deletions(-) create mode 100644 catalogs/catalog-hive/src/main/resources/hive-site.xml.template create mode 100644 catalogs/catalog-hive/src/main/resources/hive.conf 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 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 create mode 100644 catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf rename server/src/test/{java => }/resources/log4j2.properties (100%) diff --git a/build.gradle.kts b/build.gradle.kts index e9ac81b9bb..3ddc28d99a 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 66b3d0440d..d458f3d27a 100644 --- a/catalogs/catalog-hive/build.gradle.kts +++ b/catalogs/catalog-hive/build.gradle.kts @@ -90,4 +90,26 @@ 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") + + include("hive.conf") + include("hive-site.xml.template") + + rename { original -> if (original.endsWith(".template")) { + original.replace(".template", "") + } else { + original + }} + + exclude { details -> + details.file.isDirectory() + } + } + + 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 03251e44f1..f64d1dbbad 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 and pass it to hive conf + 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 0000000000..5fd225e101 --- /dev/null +++ b/catalogs/catalog-hive/src/main/resources/hive-site.xml.template @@ -0,0 +1,7 @@ + + + + diff --git a/catalogs/catalog-hive/src/main/resources/hive.conf b/catalogs/catalog-hive/src/main/resources/hive.conf new file mode 100644 index 0000000000..dee575a506 --- /dev/null +++ b/catalogs/catalog-hive/src/main/resources/hive.conf @@ -0,0 +1,12 @@ +# +# 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.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-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 0000000000..51bf05c9f9 --- /dev/null +++ b/catalogs/catalog-hive/src/test/java/com/datastrato/graviton/catalog/hive/TestHiveCatalogOperations.java @@ -0,0 +1,37 @@ +/* + * 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; + +class TestHiveCatalogOperations { + @Test + void testInitialize() { + 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); + + // Test If user properties can override the value in hive-site.xml + 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 0000000000..97626980c0 --- /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/catalogs/catalog-lakehouse-iceberg/build.gradle.kts b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts index 0bc6ca2c28..b8ed8c6b4b 100644 --- a/catalogs/catalog-lakehouse-iceberg/build.gradle.kts +++ b/catalogs/catalog-lakehouse-iceberg/build.gradle.kts @@ -90,4 +90,27 @@ 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") + + include("lakehouse-iceberg.conf") + include("hive-site.xml.template") + include("hdfs-site.xml.template") + + rename { original -> if (original.endsWith(".template")) { + original.replace(".template", "") + } else { + original + }} + + exclude { details -> + details.file.isDirectory() + } + } + + val copyLibAndConfig by registering(Copy::class) { + dependsOn(copyCatalogLibs, copyCatalogConfig) + } } 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 0000000000..231c7c4910 --- /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 0000000000..5fd225e101 --- /dev/null +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/hive-site.xml.template @@ -0,0 +1,7 @@ + + + + diff --git a/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf new file mode 100644 index 0000000000..66ace4ca34 --- /dev/null +++ b/catalogs/catalog-lakehouse-iceberg/src/main/resources/lakehouse-iceberg.conf @@ -0,0 +1,8 @@ +# +# Copyright 2023 Datastrato. +# This software is licensed under the Apache License version 2. +# + +## 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 that pass to Lakehouse-iceberg catalog. 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 0b0aec5884..3e8865e8ff 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 a25a349052..03092a74a8 100644 --- a/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java +++ b/core/src/main/java/com/datastrato/graviton/catalog/BaseCatalog.java @@ -11,6 +11,8 @@ import com.google.common.base.Preconditions; import com.google.common.collect.Maps; import java.util.Map; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; /** * The abstract base class for Catalog implementations. @@ -27,6 +29,8 @@ public abstract class BaseCatalog implements Catalog, CatalogProvider, HasPropertyMetadata { + private static final Logger LOG = LoggerFactory.getLogger(BaseCatalog.class); + private CatalogEntity entity; private Map conf; @@ -34,6 +38,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 client configurations. + 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 d414c8c732..002f318a78 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; @@ -35,12 +37,14 @@ import com.google.common.annotations.VisibleForTesting; import com.google.common.base.Preconditions; 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; 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; @@ -48,10 +52,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; @@ -214,8 +220,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; } /** @@ -238,45 +243,42 @@ 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); + // 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 = + new CatalogEntity.Builder() + .withId(uid) + .withName(ident.name()) + .withNamespace(ident.namespace()) + .withType(type) + .withProvider(provider) + .withComment(comment) + .withProperties(StringIdentifier.addToProperties(stringId, mergedConfig)) + .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 { + 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)); - wrapper.doWithPropertiesMeta( - f -> { - f.catalogPropertiesMetadata().validatePropertyForCreate(properties); + 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); @@ -445,14 +447,14 @@ private CatalogWrapper loadCatalogInternal(NameIdentifier ident) throws NoSuchCa } private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { - Map mergedConf = - mergeConf(entity.getProperties(), catalogConf(entity.name(), config)); + Map conf = entity.getProperties(); String provider = entity.getProvider(); IsolatedClassLoader classLoader; if (config.get(Configs.CATALOG_LOAD_ISOLATED)) { - String pkgPath = buildPkgPath(mergedConf, provider); - classLoader = IsolatedClassLoader.buildClassLoader(pkgPath); + String pkgPath = buildPkgPath(conf, provider); + 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. classLoader = @@ -461,7 +463,32 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { } // Load Catalog class instance - BaseCatalog catalog; + BaseCatalog catalog = createCatalogInstance(classLoader, provider); + catalog.withCatalogConf(conf).withCatalogEntity(entity); + + CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); + // Validate catalog properties and initialize the config + classLoader.withClassLoader( + cl -> { + Map configWithoutId = Maps.newHashMap(conf); + 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); + + return wrapper; + } + + private BaseCatalog createCatalogInstance(IsolatedClassLoader classLoader, String provider) { + BaseCatalog catalog; try { catalog = classLoader.withClassLoader( @@ -483,27 +510,30 @@ private CatalogWrapper createCatalogWrapper(CatalogEntity entity) { if (catalog == null) { throw new RuntimeException("Failed to load catalog with provider: " + provider); } + return catalog; + } - // Initialize the catalog - catalog = catalog.withCatalogEntity(entity).withCatalogConf(mergedConf); - CatalogWrapper wrapper = new CatalogWrapper(catalog, classLoader); + private Map loadCatalogSpecificConfig(String provider) { + if ("test".equals(provider)) { + return Maps.newHashMap(); + } - // 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; - }); + String catalogSpecificConfigFile = provider + ".conf"; + Map catalogSpecificConfig = Maps.newHashMap(); + + String fullPath = buildConfPath(provider) + File.separator + 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) { - LOG.error("Failed to load catalog '{}' properties", entity.name(), e); - throw new RuntimeException(e); + LOG.warn( + "Failed to load catalog specific configurations, file name: '{}'", + catalogSpecificConfigFile, + e); } - return wrapper; + return catalogSpecificConfig; } static Map mergeConf(Map properties, Map conf) { @@ -512,44 +542,45 @@ static Map mergeConf(Map properties, Map catalogConf(String name, Config config) { - String confPrefix = "graviton.catalog." + name + "."; - return config.getConfigsWithPrefix(confPrefix); + /** + * Build the config path from the specific provider. Usually, the configuration file is under the + * conf and conf and package are under the same directory. + */ + private String buildConfPath(String provider) { + String gravitonHome = System.getenv("GRAVITON_HOME"); + Preconditions.checkArgument(gravitonHome != null, "GRAVITON_HOME not set"); + boolean testEnv = System.getenv("GRAVITON_TEST") != null; + if (testEnv) { + return String.join( + File.separator, + gravitonHome, + "catalogs", + "catalog-" + provider, + "build", + "resources", + "main"); + } + + return String.join(File.separator, gravitonHome, "catalogs", provider, "conf"); } private String buildPkgPath(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 pkg = conf.get(Catalog.PROPERTY_PACKAGE); String pkgPath; if (pkg != null) { pkgPath = pkg; - } else if (!testEnv) { + } else if (testEnv) { + // In test, the catalog package is under the build directory. pkgPath = - gravitonHome - + File.separator - + "catalogs" - + File.separator - + provider - + File.separator - + "libs"; + String.join( + File.separator, gravitonHome, "catalogs", "catalog-" + provider, "build", "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(); + // In real environment, the catalog package is under the catalog directory. + pkgPath = String.join(File.separator, gravitonHome, "catalogs", provider, "libs"); } return 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 094ce1d4fc..f61907e643 100644 --- a/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java +++ b/core/src/main/java/com/datastrato/graviton/utils/IsolatedClassLoader.java @@ -72,28 +72,54 @@ 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); } + } - 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); - } - }); + 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 jar under the folder to classpath. + Arrays.stream(folder.listFiles()) + .filter(f -> f.getName().endsWith(".jar")) + .forEach( + f -> { + try { + classPathContents.add(f.toURI().toURL()); + } catch (MalformedURLException e) { + LOG.warn("Failed to read jar file: {}", f.getAbsolutePath(), e); + } + }); + + // Add itself to the classpath. + try { + classPathContents.add(folder.toURI().toURL()); + } catch (MalformedURLException e) { + LOG.warn("Failed to read directory: {}", folder.getAbsolutePath(), e); + } + } - return new IsolatedClassLoader(jars, Collections.emptyList(), Collections.emptyList()); + return new IsolatedClassLoader( + classPathContents, Collections.emptyList(), Collections.emptyList()); } /** Closes the class loader. */ 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 b87450d542..6aa8473ca7 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/java/com/datastrato/graviton/catalog/TestCatalogManager.java b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java index 470b1b2bd9..bb05a38d06 100644 --- a/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java +++ b/core/src/test/java/com/datastrato/graviton/catalog/TestCatalogManager.java @@ -256,6 +256,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 +267,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 da63d18b2d..0c0036e9b0 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.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; @@ -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,7 +146,6 @@ private static void createCatalog() { "comment", properties); Catalog loadCatalog = metalake.loadCatalog(NameIdentifier.of(metalakeName, catalogName)); - Assertions.assertEquals(createdCatalog, loadCatalog); catalog = loadCatalog; } 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