diff --git a/.licenserc.yaml b/.licenserc.yaml index 1d4d42d36218c4..afb8d66123913e 100644 --- a/.licenserc.yaml +++ b/.licenserc.yaml @@ -59,6 +59,16 @@ header: - "fe/fe-filesystem/fe-filesystem-spi/src/test/resources/filesystem-plugin-surface.txt" - "fe/fe-authentication/fe-authentication-spi/src/test/resources/authentication-plugin-surface.txt" - "fe/fe-core/src/test/resources/lineage-plugin-surface.txt" + # Connector plugin settings templates. build.sh seeds each connector's live + # .conf from its template verbatim (cp -n), so the template's content IS + # the file an administrator edits in plugins/connector//. Matched by name + # rather than by a **/*.conf.template glob, so a new one is a deliberate entry + # here rather than something a wildcard silently absorbs. + - "fe/fe-connector/fe-connector-hive/src/main/resources/hms.conf.template" + - "fe/fe-connector/fe-connector-iceberg/src/main/resources/iceberg.conf.template" + - "fe/fe-connector/fe-connector-jdbc/src/main/resources/jdbc.conf.template" + - "fe/fe-connector/fe-connector-paimon/src/main/resources/paimon.conf.template" + - "fe/fe-connector/fe-connector-trino/src/main/resources/trino-connector.conf.template" # Golden 4.1.3 upgrade fixtures, emitted by Gen413Fixtures running real 4.1.3 # bytecode (see the sibling PROVENANCE.txt) and stamped "do not edit by hand". # These are the generator's only non-binary outputs -- its .bin files are skipped diff --git a/build.sh b/build.sh index 36f72ef562228f..50539178292547 100755 --- a/build.sh +++ b/build.sh @@ -1082,8 +1082,17 @@ if [[ "${BUILD_FE}" -eq 1 ]]; then fi mkdir -p "${conn_plugin_target}" unzip -o "${conn_zip}" -d "${conn_plugin_target}/" + # A connector's own settings file. The zip carries only .conf.template; the live + # .conf is seeded from it here and never overwritten, so an upgrade that unzips a new + # plugin build over this directory refreshes the jars and the template but leaves whatever the + # administrator configured. Deliberately generic (globbed on *.conf.template, no connector + # named): a new connector ships a template and needs no change here. + for conn_conf_tpl in "${conn_plugin_target}"/*.conf.template; do + [ -e "${conn_conf_tpl}" ] || continue + cp -n "${conn_conf_tpl}" "${conn_conf_tpl%.template}" + done done - unset CONN_PLUGIN_DIR conn_module conn_plugin_target conn_module_dir conn_zip + unset CONN_PLUGIN_DIR conn_module conn_plugin_target conn_module_dir conn_zip conn_conf_tpl # RC-4: self-contain the paimon connector plugin for OSS. The connector sets # fs.oss.impl=com.aliyun.jindodata.oss.JindoOssFileSystem; that impl lives in the jindofs jars, diff --git a/fe/fe-connector/README.md b/fe/fe-connector/README.md index 5f142613bcdb3f..5a1dcef5e2d883 100644 --- a/fe/fe-connector/README.md +++ b/fe/fe-connector/README.md @@ -211,7 +211,23 @@ metastore/shade/cache). For a write path, the richest example is (example: `RecordingConnectorContext`). Never touch `connector-metadata-methods.txt` unless you changed the shared SPI surface itself. -14. **Packaging.** Add `src/main/assembly/plugin-zip.xml` (copy from es or +14. **Deployment-level settings** (if any). A value that is one-per-FE rather + than one-per-catalog goes in your plugin's own settings file, NOT in + fe.conf: ship `src/main/resources/.conf.template`, add it to your + assembly's `` at the zip root, and read it with + `ConnectorConf.get(context, "", null, "")`. `` is + `ConnectorProvider.name()`, which is **not** necessarily your plugin + directory name — `plugins/connector/hive/` holds `hms.conf` and + `plugins/connector/trino/` holds `trino-connector.conf`. Guard that with a + test asserting `name() + ".conf.template"` is on the classpath (copy + `IcebergConnectorConfTest#theConfTemplateIsNamedAfterTheProvider`); a + template under any other name deploys a file the engine never opens. + `build.sh` seeds the live `.conf` from the template generically, so it + needs no change. Do NOT add a key to `Config.java` or to + `DefaultConnectorContext.buildEnvironment` — that is an engine change per + setting, and the keys still there are only the ones several connectors + share plus the fe.conf fallbacks kept for existing deployments. +15. **Packaging.** Add `src/main/assembly/plugin-zip.xml` (copy from es or paimon). Verify your module through `package`/`install`, not just `test-compile` — shades and the plugin zip only materialize then. 15. **Gates and e2e.** Your module must pass the forbidden-import gate (runs diff --git a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/package-info.java b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/package-info.java index bbc502ae22d627..a0e28e0bb43c81 100644 --- a/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/package-info.java +++ b/fe/fe-connector/fe-connector-api/src/main/java/org/apache/doris/connector/api/package-info.java @@ -160,8 +160,8 @@ * *

Rule 7 — where a connector's tunable knobs live

* - *

Pick the channel by the SCOPE of the value. Only the second one obliges anyone to touch the engine, so - * a knob that can be catalog-scoped should be.

+ *

Pick the channel by the SCOPE of the value. None of them requires an engine change any more, so a + * knob that can be catalog-scoped should be, and one that cannot belongs in the plugin's own conf file.

* *
    *
  • Per catalog → a key in {@code CREATE CATALOG ... PROPERTIES(...)}. The engine hands the @@ -174,9 +174,22 @@ * declared in {@code HiveConnectorProperties} and read in {@code HiveScanPlanProvider} / * {@code HiveConnector}; those key strings appear nowhere in {@code fe-core}.
    • *
  • Per FE process (one deployment-level value for every catalog, e.g. a driver directory) - * → an {@code fe.conf} field forwarded through {@code ConnectorContext.getEnvironment()} by - * {@code DefaultConnectorContext.buildEnvironment}. This is the one knob shape that requires an engine - * change per key, so use it only when the value genuinely is not per catalog.
    • + * → a key in the plugin's own {@code .conf}, read with {@code ConnectorConf.get}. The engine + * locates and parses {@code /.conf} generically + * ({@code ConnectorPluginManager.loadPlugins}) and serves it back through + * {@code ConnectorContext.getConnectorConfig()}, so no key name of yours reaches {@code fe-core} and + * adding one costs the engine nothing. Do not prefix these keys — the file name already + * namespaces them. Ship {@code src/main/resources/.conf.template} and add it to your assembly's + * {@code } at the zip root; {@code build.sh} seeds the live {@code .conf} from it. {@code } + * is {@code ConnectorProvider.name()} and need not equal your plugin directory name + * ({@code plugins/connector/hive/} holds {@code hms.conf}). + *
  • Per FE process, legacy → an {@code fe.conf} field forwarded through + * {@code ConnectorContext.getEnvironment()} by {@code DefaultConnectorContext.buildEnvironment}. This is + * the one shape that requires an engine change per key, and it is closed to new keys. What is + * still there is either shared by several connectors or not a connector setting at all + * ({@code doris_home}, {@code doris_version}); the connector settings among them are kept as the + * fallback {@code ConnectorConf.get} consults after the plugin conf, so deployments configured before + * the conf files existed keep working untouched.
    • *
  • Per session → read the query's session variables from * {@link ConnectorSession#getSessionProperties()}. The connector does not declare them; it looks up the * names it cares about.
    • diff --git a/fe/fe-connector/fe-connector-hive/src/main/assembly/plugin-zip.xml b/fe/fe-connector/fe-connector-hive/src/main/assembly/plugin-zip.xml index 2e141b72f15096..89d021ed83c3ba 100644 --- a/fe/fe-connector/fe-connector-hive/src/main/assembly/plugin-zip.xml +++ b/fe/fe-connector/fe-connector-hive/src/main/assembly/plugin-zip.xml @@ -40,6 +40,17 @@ under the License. ${project.build.directory}/${project.build.finalName}.jar / + + + src/main/resources/hms.conf.template + / +     diff --git a/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorMetadata.java b/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorMetadata.java index 261eab5a67ca3d..603c2071e23355 100644 --- a/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorMetadata.java +++ b/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorMetadata.java @@ -67,6 +67,7 @@ import org.apache.doris.connector.hms.HmsPartitionInfo; import org.apache.doris.connector.hms.HmsTableInfo; import org.apache.doris.connector.hms.HmsTypeMapping; +import org.apache.doris.connector.spi.ConnectorConf; import org.apache.doris.connector.spi.ConnectorContext; import org.apache.doris.connector.spi.ConnectorStorageContext; import org.apache.doris.filesystem.FileSystem; @@ -1618,7 +1619,8 @@ public void createTable(ConnectorSession session, ConnectorCreateTableRequest re } Map env = context.getEnvironment(); String fileFormat = userProps.getOrDefault(HiveConnectorProperties.CREATE_FILE_FORMAT, - env.getOrDefault(HiveConnectorProperties.ENV_HIVE_DEFAULT_FILE_FORMAT, + ConnectorConf.get(context, HiveConnectorProperties.CONF_DEFAULT_FILE_FORMAT, + HiveConnectorProperties.ENV_HIVE_DEFAULT_FILE_FORMAT, HiveConnectorProperties.DEFAULT_FILE_FORMAT)); // Metastore table parameters: lower-case every key and stamp the file_format / location keys under a @@ -1673,11 +1675,14 @@ public void createTable(ConnectorSession session, ConnectorCreateTableRequest re // enable gate first, then the hash requirement. ConnectorBucketSpec bucketSpec = request.getBucketSpec(); if (bucketSpec != null) { - boolean bucketEnabled = Boolean.parseBoolean(env.getOrDefault( + boolean bucketEnabled = Boolean.parseBoolean(ConnectorConf.get(context, + HiveConnectorProperties.CONF_ENABLE_CREATE_BUCKET_TABLE, HiveConnectorProperties.ENV_ENABLE_CREATE_HIVE_BUCKET_TABLE, "false")); if (!bucketEnabled) { - throw new DorisConnectorException( - "Create hive bucket table need set enable_create_hive_bucket_table to true"); + throw new DorisConnectorException("Create hive bucket table need set '" + + HiveConnectorProperties.CONF_ENABLE_CREATE_BUCKET_TABLE + "' in hms.conf (or " + + HiveConnectorProperties.ENV_ENABLE_CREATE_HIVE_BUCKET_TABLE + + " in fe.conf) to true"); } if (HiveConnectorProperties.BUCKET_ALGO_RANDOM.equals(bucketSpec.getAlgorithm())) { throw new DorisConnectorException("External hive table only supports hash bucketing"); diff --git a/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorProperties.java b/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorProperties.java index dae9f256b0d62b..590a26f9c3d23a 100644 --- a/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorProperties.java +++ b/fe/fe-connector/fe-connector-hive/src/main/java/org/apache/doris/connector/hive/HiveConnectorProperties.java @@ -73,6 +73,13 @@ private HiveConnectorProperties() { new HashSet<>(Arrays.asList(CREATE_FILE_FORMAT, CREATE_LOCATION))); public static final String DORIS_PROP_PREFIX = "doris."; + // -- deployment-level settings, read from this plugin's own hms.conf -- + // The file is named after ConnectorProvider.name() ("hms"), NOT after the plugin directory + // ("plugins/connector/hive"). Each key falls back to the ENV_ name below it, which is the fe.conf key + // it used to live under and still works. + public static final String CONF_DEFAULT_FILE_FORMAT = "default_file_format"; + public static final String CONF_ENABLE_CREATE_BUCKET_TABLE = "enable_create_bucket_table"; + // -- environment keys threaded from fe-core DefaultConnectorContext (must stay byte-identical there) -- public static final String ENV_HIVE_DEFAULT_FILE_FORMAT = "hive_default_file_format"; public static final String ENV_ENABLE_CREATE_HIVE_BUCKET_TABLE = "enable_create_hive_bucket_table"; diff --git a/fe/fe-connector/fe-connector-hive/src/main/resources/hms.conf.template b/fe/fe-connector/fe-connector-hive/src/main/resources/hms.conf.template new file mode 100644 index 00000000000000..e4597e0188b662 --- /dev/null +++ b/fe/fe-connector/fe-connector-hive/src/main/resources/hms.conf.template @@ -0,0 +1,21 @@ +# Hive (HMS) connector plugin configuration. +# +# build.sh seeds hms.conf from this file on first deploy and never overwrites it, so an upgrade that +# unzips a new plugin build over this directory keeps whatever you configured here. +# This file must exist on EVERY FE node -- it is not replicated through Doris metadata. +# Changes take effect after an FE restart. +# +# The file name is the connector's name (ConnectorProvider.name() == "hms"), NOT the directory name: +# this plugin is deployed under plugins/connector/hive/ but its settings file is hms.conf. +# +# Every setting below is optional. Left commented out, each falls back to the fe.conf key named +# after it, and then to the built-in default -- so an existing deployment needs no change here. + +# Storage format for a CREATE TABLE that does not name one itself. +# Falls back to fe.conf's hive_default_file_format, whose default is orc. A CREATE TABLE may override +# it per table with the file_format property, which wins over both. +# default_file_format=orc + +# Whether CREATE TABLE may create a bucketed hive table. Off by default. +# Falls back to fe.conf's enable_create_hive_bucket_table. +# enable_create_bucket_table=false diff --git a/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/FakeConnectorContext.java b/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/FakeConnectorContext.java index 30b7205c4c2268..7c135b2427c50c 100644 --- a/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/FakeConnectorContext.java +++ b/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/FakeConnectorContext.java @@ -24,9 +24,10 @@ import java.util.Map; /** - * Minimal {@link ConnectorContext} test double: carries a fixed catalog identity and an environment map (the - * channel through which fe-core threads the FE-global CREATE TABLE defaults). Everything else uses the - * interface defaults. + * Minimal {@link ConnectorContext} test double: carries a fixed catalog identity plus the two maps a + * deployment-level setting can arrive in — this plugin's own {@code hms.conf} + * ({@link #getConnectorConfig()}) and fe.conf as forwarded by the engine ({@link #getEnvironment()}). + * Everything else uses the interface defaults. */ public class FakeConnectorContext implements ConnectorContext, ConnectorStorageContext { @@ -40,6 +41,7 @@ public ConnectorStorageContext getStorageContext() { private final String catalogName; private final long catalogId; private final Map environment; + private final Map connectorConfig; public FakeConnectorContext() { this("test_catalog", 0L, Collections.emptyMap()); @@ -50,9 +52,15 @@ public FakeConnectorContext(Map environment) { } public FakeConnectorContext(String catalogName, long catalogId, Map environment) { + this(catalogName, catalogId, environment, Collections.emptyMap()); + } + + public FakeConnectorContext(String catalogName, long catalogId, Map environment, + Map connectorConfig) { this.catalogName = catalogName; this.catalogId = catalogId; this.environment = environment == null ? Collections.emptyMap() : environment; + this.connectorConfig = connectorConfig == null ? Collections.emptyMap() : connectorConfig; } @Override @@ -69,4 +77,9 @@ public long getCatalogId() { public Map getEnvironment() { return environment; } + + @Override + public Map getConnectorConfig() { + return connectorConfig; + } } diff --git a/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/HiveConnectorMetadataDdlTest.java b/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/HiveConnectorMetadataDdlTest.java index 2afd824d91b777..3ff1414048ab1a 100644 --- a/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/HiveConnectorMetadataDdlTest.java +++ b/fe/fe-connector/fe-connector-hive/src/test/java/org/apache/doris/connector/hive/HiveConnectorMetadataDdlTest.java @@ -112,6 +112,67 @@ public void createTableFallsBackToOrcWhenEnvMissing() { Assertions.assertEquals("orc", client.lastCreateTable.getFileFormat()); } + // ==================== createTable: the plugin's own hms.conf ==================== + + @Test + public void createTableFileFormatFromPluginConfBeatsFeConf() { + RecordingHmsClient client = new RecordingHmsClient(); + // WHY: the point of the plugin conf channel. An administrator who sets default_file_format in + // hms.conf must get it even though fe.conf still names the old value -- reverse the precedence + // and migrating a deployment to the new file silently does nothing. + Map conf = Collections.singletonMap( + HiveConnectorProperties.CONF_DEFAULT_FILE_FORMAT, "parquet"); + Map env = Collections.singletonMap( + HiveConnectorProperties.ENV_HIVE_DEFAULT_FILE_FORMAT, "orc"); + + metadataWithConf(client, conf, env).createTable(session(), request().build()); + + Assertions.assertEquals("parquet", client.lastCreateTable.getFileFormat()); + } + + @Test + public void createTableUserFileFormatStillBeatsThePluginConf() { + RecordingHmsClient client = new RecordingHmsClient(); + // WHY: the new channel is deployment-level; it must not outrank a per-catalog CREATE TABLE + // property. Only the two deployment channels reordered relative to each other. + Map conf = Collections.singletonMap( + HiveConnectorProperties.CONF_DEFAULT_FILE_FORMAT, "parquet"); + + metadataWithConf(client, conf, Collections.emptyMap()).createTable(session(), + request().properties(Collections.singletonMap("file_format", "orc")).build()); + + Assertions.assertEquals("orc", client.lastCreateTable.getFileFormat()); + } + + @Test + public void createTableBucketGateCanBeOpenedFromThePluginConfAlone() { + RecordingHmsClient client = new RecordingHmsClient(); + // WHY: the second migrated setting, and the one where getting the channel wrong is expensive -- + // a deployment that opts in through hms.conf but is still gated by fe.conf's default 'false' + // would find bucketed creates rejected with no indication which file is in charge. + Map conf = Collections.singletonMap( + HiveConnectorProperties.CONF_ENABLE_CREATE_BUCKET_TABLE, "true"); + ConnectorBucketSpec bucket = new ConnectorBucketSpec( + Collections.singletonList("id"), 8, "doris_default"); + + metadataWithConf(client, conf, Collections.emptyMap()) + .createTable(session(), request().bucketSpec(bucket).build()); + + Assertions.assertEquals(Collections.singletonList("id"), client.lastCreateTable.getBucketCols()); + Assertions.assertEquals(8, client.lastCreateTable.getNumBuckets()); + } + + @Test + public void theConfTemplateIsNamedAfterTheProvider() { + // WHY: the engine reads .conf, and this connector's name is "hms" while its plugin + // directory is "hive". A template under any other name deploys a file nothing ever opens -- + // silently, with every setting in it ignored. Renaming getType() must break here. + String expected = new HiveConnectorProvider().name() + ".conf.template"; + Assertions.assertNotNull( + HiveConnectorMetadataDdlTest.class.getClassLoader().getResource(expected), + "the plugin must ship " + expected + " on its classpath"); + } + // ==================== createTable: transactional rejection ==================== @Test @@ -368,6 +429,12 @@ private static HiveConnectorMetadata metadata(RecordingHmsClient client, return new HiveConnectorMetadata(client, catalogProps, new FakeConnectorContext(env)); } + private static HiveConnectorMetadata metadataWithConf(RecordingHmsClient client, + Map conf, Map env) { + return new HiveConnectorMetadata(client, Collections.emptyMap(), + new FakeConnectorContext("test_catalog", 0L, env, conf)); + } + /** * Columns are nullable: hive rejects a {@code NOT NULL} column up front (validateColumns runs before every * other createTable check), so a NOT NULL fixture column would short-circuit every test in this class before diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/assembly/plugin-zip.xml b/fe/fe-connector/fe-connector-iceberg/src/main/assembly/plugin-zip.xml index 9c46915cc7bf65..c8dd917dc9361c 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/assembly/plugin-zip.xml +++ b/fe/fe-connector/fe-connector-iceberg/src/main/assembly/plugin-zip.xml @@ -36,6 +36,20 @@ under the License. + + + + src/main/resources/iceberg.conf.template + / + + + lib diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java index cd1ffb5f9b87bf..738cf7bd1bf7bb 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnector.java @@ -34,6 +34,7 @@ import org.apache.doris.connector.metastore.HmsMetaStoreProperties; import org.apache.doris.connector.metastore.spi.JdbcDriverSupport; import org.apache.doris.connector.metastore.spi.MetaStoreProviders; +import org.apache.doris.connector.spi.ConnectorConf; import org.apache.doris.connector.spi.ConnectorContext; import org.apache.doris.connector.spi.ConnectorStorageContext; import org.apache.doris.filesystem.properties.S3CompatibleFileSystemProperties; @@ -922,8 +923,10 @@ private Catalog createCatalog() { IcebergConnectorProperties.TYPE_HMS, properties, storageHadoopConfig); conf = IcebergCatalogFactory.assembleHiveConf( IcebergCatalogFactory.firstNonBlank(properties, "hive.conf.resources"), - hms.toHiveConfOverrides(context.getEnvironment() - .getOrDefault("hive_metastore_client_timeout_second", "10"))); + hms.toHiveConfOverrides(ConnectorConf.get(context, + IcebergConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND))); break; } case IcebergConnectorProperties.TYPE_GLUE: @@ -1407,13 +1410,32 @@ private void maybeRegisterJdbcDriver() { LOG.info("Using dynamic JDBC driver for Iceberg JDBC catalog from: {}", driverUrl); } + /** + * The directory a bare driver jar name resolves under, from this plugin's own {@code iceberg.conf} + * or fe.conf's {@code jdbc_drivers_dir}. Null when neither names one — {@code JdbcDriverSupport} + * then falls back to {@code /plugins/jdbc_drivers}, as before. + * + *

    A null context is a direct-construction unit test, which has neither channel. + */ + private String configuredDriversDir() { + return context == null ? null : ConnectorConf.get(context, + IcebergConnectorProperties.CONF_DRIVERS_DIR, + IcebergConnectorProperties.ENV_JDBC_DRIVERS_DIR, null); + } + + /** The FE install root. Engine-wide rather than this connector's, so it stays in the environment. */ + private String configuredDorisHome() { + return context == null ? null + : context.getEnvironment().get(IcebergConnectorProperties.ENV_DORIS_HOME); + } + private void registerJdbcDriver(String driverUrl, String driverClassName) { try { if (StringUtils.isBlank(driverClassName)) { throw new IllegalArgumentException("driver_class is required when driver_url is specified"); } - Map env = context != null ? context.getEnvironment() : Collections.emptyMap(); - String fullDriverUrl = JdbcDriverSupport.resolveDriverUrl(driverUrl, env); + String fullDriverUrl = JdbcDriverSupport.resolveDriverUrl(driverUrl, + configuredDriversDir(), configuredDorisHome()); URL url = new URL(fullDriverUrl); String driverKey = fullDriverUrl + "#" + driverClassName; if (!REGISTERED_DRIVER_KEYS.add(driverKey)) { diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorProperties.java b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorProperties.java index f2febaf2e432ba..ac8289cb022256 100644 --- a/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorProperties.java +++ b/fe/fe-connector/fe-connector-iceberg/src/main/java/org/apache/doris/connector/iceberg/IcebergConnectorProperties.java @@ -27,6 +27,27 @@ public final class IcebergConnectorProperties { private IcebergConnectorProperties() { } + // -- Deployment-level settings, read from this plugin's own iceberg.conf (named after + // ConnectorProvider.name()). Each falls back to the ENV_ name below it, which is the fe.conf key it + // used to live under and still works. + // + // Both are shared with other connectors at the fe.conf end -- one jdbc_drivers_dir and one + // hive_metastore_client_timeout_second serve jdbc, iceberg and paimon. A plugin conf cannot express + // that, so a deployment that moves to these files sets the value in each plugin's own conf. That is + // the accepted cost of a per-plugin file; the fe.conf keys stay as the shared fallback. -- + public static final String CONF_DRIVERS_DIR = "drivers_dir"; + public static final String CONF_METASTORE_CLIENT_TIMEOUT_SECOND = "metastore_client_timeout_second"; + + /** The fe.conf name of {@link #CONF_DRIVERS_DIR}, forwarded through the engine environment. */ + public static final String ENV_JDBC_DRIVERS_DIR = "jdbc_drivers_dir"; + /** The fe.conf name of {@link #CONF_METASTORE_CLIENT_TIMEOUT_SECOND}. */ + public static final String ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND = + "hive_metastore_client_timeout_second"; + /** Engine-wide, not this connector's: the FE install root. Stays in the engine environment. */ + public static final String ENV_DORIS_HOME = "doris_home"; + /** Legacy default when neither channel names a metastore client timeout. */ + public static final String DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND = "10"; + // -- Catalog type (second-level dispatch) -- public static final String ICEBERG_CATALOG_TYPE = "iceberg.catalog.type"; diff --git a/fe/fe-connector/fe-connector-iceberg/src/main/resources/iceberg.conf.template b/fe/fe-connector/fe-connector-iceberg/src/main/resources/iceberg.conf.template new file mode 100644 index 00000000000000..668c9328ac3271 --- /dev/null +++ b/fe/fe-connector/fe-connector-iceberg/src/main/resources/iceberg.conf.template @@ -0,0 +1,23 @@ +# Iceberg connector plugin configuration. +# +# build.sh seeds iceberg.conf from this file on first deploy and never overwrites it, so an upgrade +# that unzips a new plugin build over this directory keeps whatever you configured here. +# This file must exist on EVERY FE node -- it is not replicated through Doris metadata. +# Changes take effect after an FE restart. +# +# Every setting below is optional. Left commented out, each falls back to the fe.conf key named +# after it, and then to the built-in default -- so an existing deployment needs no change here. +# +# NOTE: both settings below are shared with other connectors at the fe.conf end -- one +# jdbc_drivers_dir and one hive_metastore_client_timeout_second serve jdbc, iceberg and paimon. +# A per-plugin conf cannot express that, so if you move them here you must also set them in +# jdbc.conf and paimon.conf. Leaving them commented out keeps the single shared fe.conf value. + +# Directory a bare driver jar name in iceberg.jdbc.driver_url resolves under (JDBC catalog only). +# Falls back to fe.conf's jdbc_drivers_dir, whose default is /plugins/jdbc_drivers. +# drivers_dir=/opt/doris/plugins/jdbc_drivers + +# Socket timeout, in seconds, for this catalog's Hive metastore client (HMS catalog only), used +# unless the catalog overrides it. Falls back to fe.conf's hive_metastore_client_timeout_second, +# whose default is 10. +# metastore_client_timeout_second=10 diff --git a/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorConfTest.java b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorConfTest.java new file mode 100644 index 00000000000000..4fd6e69b213e76 --- /dev/null +++ b/fe/fe-connector/fe-connector-iceberg/src/test/java/org/apache/doris/connector/iceberg/IcebergConnectorConfTest.java @@ -0,0 +1,124 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.iceberg; + +import org.apache.doris.connector.spi.ConnectorConf; +import org.apache.doris.connector.spi.ConnectorContext; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +/** + * How this connector's two deployment-level settings are resolved: its own {@code iceberg.conf} + * first, then the fe.conf key each used to live under. + * + *

    Asserted rather than trusted, because both failure directions are silent. Reading fe.conf first + * would make an administrator's edit to iceberg.conf do nothing. Dropping the fe.conf fallback would + * change an untouched deployment's metastore timeout and drivers directory on upgrade, with nothing + * in either file to show why. + */ +public class IcebergConnectorConfTest { + + @Test + public void driversDirPrefersThePluginConfThenFeConf() { + Assertions.assertEquals("/from/plugin/conf", ConnectorConf.get( + context(Collections.singletonMap(IcebergConnectorProperties.CONF_DRIVERS_DIR, + "/from/plugin/conf"), + Collections.singletonMap(IcebergConnectorProperties.ENV_JDBC_DRIVERS_DIR, + "/from/fe/conf")), + IcebergConnectorProperties.CONF_DRIVERS_DIR, + IcebergConnectorProperties.ENV_JDBC_DRIVERS_DIR, null)); + + Assertions.assertEquals("/from/fe/conf", ConnectorConf.get( + context(Collections.emptyMap(), + Collections.singletonMap(IcebergConnectorProperties.ENV_JDBC_DRIVERS_DIR, + "/from/fe/conf")), + IcebergConnectorProperties.CONF_DRIVERS_DIR, + IcebergConnectorProperties.ENV_JDBC_DRIVERS_DIR, null)); + + Assertions.assertNull(ConnectorConf.get(context(Collections.emptyMap(), Collections.emptyMap()), + IcebergConnectorProperties.CONF_DRIVERS_DIR, + IcebergConnectorProperties.ENV_JDBC_DRIVERS_DIR, null)); + } + + @Test + public void metastoreTimeoutPrefersThePluginConfThenFeConfThenTen() { + Assertions.assertEquals("30", ConnectorConf.get( + context(Collections.singletonMap( + IcebergConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, "30"), + Collections.singletonMap( + IcebergConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, "20")), + IcebergConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND)); + + Assertions.assertEquals("20", ConnectorConf.get( + context(Collections.emptyMap(), Collections.singletonMap( + IcebergConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, "20")), + IcebergConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND)); + + // The literal the call site used before this channel existed; keeping it is what makes a + // deployment with neither file behave exactly as it did. + Assertions.assertEquals("10", ConnectorConf.get( + context(Collections.emptyMap(), Collections.emptyMap()), + IcebergConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + IcebergConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND)); + } + + @Test + public void theConfTemplateIsNamedAfterTheProvider() { + // The engine reads .conf, so a template under any other name deploys a file nothing ever + // opens -- silently, with every setting in it ignored. Renaming getType() must break here. + String expected = new IcebergConnectorProvider().name() + ".conf.template"; + Assertions.assertNotNull(getClass().getClassLoader().getResource(expected), + "the plugin must ship " + expected + " on its classpath"); + } + + private static ConnectorContext context(Map conf, Map env) { + Map confCopy = new HashMap<>(conf); + Map envCopy = new HashMap<>(env); + return new ConnectorContext() { + @Override + public String getCatalogName() { + return "test_catalog"; + } + + @Override + public long getCatalogId() { + return 1L; + } + + @Override + public Map getConnectorConfig() { + return confCopy; + } + + @Override + public Map getEnvironment() { + return envCopy; + } + }; + } +} diff --git a/fe/fe-connector/fe-connector-jdbc/src/main/assembly/plugin-zip.xml b/fe/fe-connector/fe-connector-jdbc/src/main/assembly/plugin-zip.xml index 2e141b72f15096..d0f53bf68d6833 100644 --- a/fe/fe-connector/fe-connector-jdbc/src/main/assembly/plugin-zip.xml +++ b/fe/fe-connector/fe-connector-jdbc/src/main/assembly/plugin-zip.xml @@ -40,6 +40,16 @@ under the License. ${project.build.directory}/${project.build.finalName}.jar / + + + src/main/resources/jdbc.conf.template + / + diff --git a/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcConnectorProperties.java b/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcConnectorProperties.java index f3a3e063133453..6641eec5ed1c0b 100644 --- a/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcConnectorProperties.java +++ b/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcConnectorProperties.java @@ -30,6 +30,19 @@ private JdbcConnectorProperties() { } // -- connection -- + // -- deployment-level settings, read from this plugin's own jdbc.conf (named after + // ConnectorProvider.name()). Each falls back to the ENV_ name below it, which is the fe.conf key it + // used to live under and still works. -- + public static final String CONF_DRIVERS_DIR = "drivers_dir"; + public static final String CONF_FORCE_SQLSERVER_ENCRYPT_FALSE = "force_sqlserver_encrypt_false"; + + /** The fe.conf name of {@link #CONF_DRIVERS_DIR}, forwarded through the engine environment. */ + public static final String ENV_DRIVERS_DIR = "jdbc_drivers_dir"; + /** The fe.conf name of {@link #CONF_FORCE_SQLSERVER_ENCRYPT_FALSE}. */ + public static final String ENV_FORCE_SQLSERVER_ENCRYPT_FALSE = "force_sqlserver_jdbc_encrypt_false"; + /** Engine-wide, not this connector's: the FE install root. Stays in the engine environment. */ + public static final String ENV_DORIS_HOME = "doris_home"; + public static final String JDBC_URL = "jdbc_url"; public static final String USER = "user"; public static final String PASSWORD = "password"; diff --git a/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcDorisConnector.java b/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcDorisConnector.java index 885f19a7a046c0..01e1b94df8a6d4 100644 --- a/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcDorisConnector.java +++ b/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcDorisConnector.java @@ -27,6 +27,7 @@ import org.apache.doris.connector.api.scan.ConnectorScanPlanProvider; import org.apache.doris.connector.api.write.ConnectorWritePlanProvider; import org.apache.doris.connector.jdbc.client.JdbcConnectorClient; +import org.apache.doris.connector.spi.ConnectorConf; import org.apache.doris.connector.spi.ConnectorContext; import org.apache.doris.thrift.TJdbcTable; import org.apache.doris.thrift.TOdbcTableType; @@ -86,7 +87,11 @@ public JdbcDorisConnector(Map properties, ConnectorContext conte if (rawUrl != null && !rawUrl.isEmpty()) { JdbcDbType dbType = JdbcDbType.parseFromUrl(rawUrl); normalized.put(JdbcConnectorProperties.JDBC_URL, - JdbcUrlNormalizer.normalize(rawUrl, dbType, context.getEnvironment())); + JdbcUrlNormalizer.normalize(rawUrl, dbType, + Boolean.parseBoolean(ConnectorConf.get(context, + JdbcConnectorProperties.CONF_FORCE_SQLSERVER_ENCRYPT_FALSE, + JdbcConnectorProperties.ENV_FORCE_SQLSERVER_ENCRYPT_FALSE, + "false")))); } this.properties = Collections.unmodifiableMap(normalized); this.context = context; @@ -319,9 +324,10 @@ public void close() throws IOException { } /** - * Resolves driver URL using the environment from ConnectorContext. + * Resolves driver URL against the configured drivers directory. * If the URL is a plain filename (e.g., "mysql-connector-j-8.4.0.jar"), - * resolves it using the jdbc_drivers_dir from the environment. + * resolves it under {@code drivers_dir} from this plugin's jdbc.conf, or fe.conf's + * {@code jdbc_drivers_dir}. */ private String resolveDriverUrl(String driverUrl) { if (driverUrl == null || driverUrl.isEmpty()) { @@ -331,10 +337,11 @@ private String resolveDriverUrl(String driverUrl) { || driverUrl.startsWith("https://") || driverUrl.startsWith("/")) { return driverUrl; } - // Plain filename — resolve using jdbc_drivers_dir from environment - Map env = context.getEnvironment(); - String driversDir = env.get("jdbc_drivers_dir"); - String dorisHome = env.get("doris_home"); + // Plain filename — resolve under the configured drivers directory. doris_home is engine-wide + // rather than this connector's setting, so it keeps coming from the engine environment. + String driversDir = ConnectorConf.get(context, JdbcConnectorProperties.CONF_DRIVERS_DIR, + JdbcConnectorProperties.ENV_DRIVERS_DIR, null); + String dorisHome = context.getEnvironment().get(JdbcConnectorProperties.ENV_DORIS_HOME); if (driversDir != null && !driversDir.isEmpty()) { String newPath = driversDir + "/" + driverUrl; if (new File(newPath).exists()) { diff --git a/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizer.java b/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizer.java index 50987f308d2e1e..b5eea3587495a0 100644 --- a/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizer.java +++ b/fe/fe-connector/fe-connector-jdbc/src/main/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizer.java @@ -17,9 +17,6 @@ package org.apache.doris.connector.jdbc; -import java.util.Collections; -import java.util.Map; - /** * Normalizes JDBC URLs by adding required parameters for correct behavior. * Replicates the logic from {@code JdbcResource.handleJdbcUrl()} in fe-core, @@ -55,21 +52,25 @@ private JdbcUrlNormalizer() { *

    For SQL Server: *

      *
    • {@code useBulkCopyForBatchInsert=true}
      • - *
    • {@code encrypt=false} — when {@code force_sqlserver_jdbc_encrypt_false} is set
      • + *
    • {@code encrypt=false} — when the deployment asks for it
      • *
    */ public static String normalize(String jdbcUrl, JdbcDbType dbType) { - return normalize(jdbcUrl, dbType, Collections.emptyMap()); + return normalize(jdbcUrl, dbType, false); } /** - * Normalize a JDBC URL with engine environment context. + * Normalize a JDBC URL, honoring the deployment-level SQL Server encryption override. * * @param jdbcUrl the raw JDBC URL * @param dbType the database type - * @param environment engine environment properties (from ConnectorContext) + * @param forceSqlServerEncryptFalse whether to pin {@code encrypt=false} on a SQL Server URL. + * Resolved by the caller from {@code force_sqlserver_encrypt_false} + * in the plugin's own jdbc.conf or + * {@code force_sqlserver_jdbc_encrypt_false} in fe.conf */ - public static String normalize(String jdbcUrl, JdbcDbType dbType, Map environment) { + public static String normalize(String jdbcUrl, JdbcDbType dbType, + boolean forceSqlServerEncryptFalse) { if (jdbcUrl == null || jdbcUrl.isEmpty()) { return jdbcUrl; } @@ -95,8 +96,7 @@ public static String normalize(String jdbcUrl, JdbcDbType dbType, Map/plugins/jdbc_drivers. +# drivers_dir=/opt/doris/plugins/jdbc_drivers + +# Pin encrypt=false on SQL Server JDBC URLs. Off by default. +# Falls back to fe.conf's force_sqlserver_jdbc_encrypt_false. +# force_sqlserver_encrypt_false=false diff --git a/fe/fe-connector/fe-connector-jdbc/src/test/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizerTest.java b/fe/fe-connector/fe-connector-jdbc/src/test/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizerTest.java index c19dc3ba941f8a..a525ad1c88bf90 100644 --- a/fe/fe-connector/fe-connector-jdbc/src/test/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizerTest.java +++ b/fe/fe-connector/fe-connector-jdbc/src/test/java/org/apache/doris/connector/jdbc/JdbcUrlNormalizerTest.java @@ -17,12 +17,17 @@ package org.apache.doris.connector.jdbc; +import org.apache.doris.connector.spi.ConnectorConf; +import org.apache.doris.connector.spi.ConnectorContext; + import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; +import java.util.Map; + /** * Tests for {@link JdbcUrlNormalizer}, focusing on the setParamIfAbsent - * duplicate-append fix (P1-7). + * duplicate-append fix (P1-7), plus how this connector's two deployment-level settings are resolved. */ public class JdbcUrlNormalizerTest { @@ -118,20 +123,16 @@ void testUnknownDbTypeReturnsUrlUnchanged() { @Test void testSqlServerEncryptOverrideWhenForced() { - java.util.Map env = java.util.Map.of( - "force_sqlserver_jdbc_encrypt_false", "true"); String url = "jdbc:sqlserver://host:1433;databaseName=test"; - String result = JdbcUrlNormalizer.normalize(url, JdbcDbType.SQLSERVER, env); + String result = JdbcUrlNormalizer.normalize(url, JdbcDbType.SQLSERVER, true); Assertions.assertTrue(result.contains(";encrypt=false"), - "encrypt=false should be added when force_sqlserver_jdbc_encrypt_false is true; got: " + result); + "encrypt=false should be added when the override is on; got: " + result); } @Test void testSqlServerEncryptOverrideReplacesTrue() { - java.util.Map env = java.util.Map.of( - "force_sqlserver_jdbc_encrypt_false", "true"); String url = "jdbc:sqlserver://host:1433;encrypt=true;databaseName=test"; - String result = JdbcUrlNormalizer.normalize(url, JdbcDbType.SQLSERVER, env); + String result = JdbcUrlNormalizer.normalize(url, JdbcDbType.SQLSERVER, true); Assertions.assertTrue(result.contains("encrypt=false"), "encrypt=true should be replaced with encrypt=false; got: " + result); Assertions.assertFalse(result.contains("encrypt=true"), @@ -146,6 +147,75 @@ void testSqlServerEncryptNotOverriddenByDefault() { "encrypt=false should NOT be added without force flag; got: " + result); } + @Test + void theEncryptOverrideIsReadFromThePluginConfFirstThenFeConf() { + // The resolution the connector performs before calling normalize. Asserted here rather than + // trusted: an administrator who turns the override on in jdbc.conf must not be silently + // overruled by fe.conf's default, and vice versa an untouched deployment must keep reading + // fe.conf exactly as before. + Assertions.assertEquals("true", ConnectorConf.get( + context(Map.of(JdbcConnectorProperties.CONF_FORCE_SQLSERVER_ENCRYPT_FALSE, "true"), + Map.of(JdbcConnectorProperties.ENV_FORCE_SQLSERVER_ENCRYPT_FALSE, "false")), + JdbcConnectorProperties.CONF_FORCE_SQLSERVER_ENCRYPT_FALSE, + JdbcConnectorProperties.ENV_FORCE_SQLSERVER_ENCRYPT_FALSE, "false")); + + Assertions.assertEquals("true", ConnectorConf.get( + context(Map.of(), Map.of(JdbcConnectorProperties.ENV_FORCE_SQLSERVER_ENCRYPT_FALSE, "true")), + JdbcConnectorProperties.CONF_FORCE_SQLSERVER_ENCRYPT_FALSE, + JdbcConnectorProperties.ENV_FORCE_SQLSERVER_ENCRYPT_FALSE, "false")); + + Assertions.assertEquals("false", ConnectorConf.get(context(Map.of(), Map.of()), + JdbcConnectorProperties.CONF_FORCE_SQLSERVER_ENCRYPT_FALSE, + JdbcConnectorProperties.ENV_FORCE_SQLSERVER_ENCRYPT_FALSE, "false")); + } + + @Test + void theDriversDirIsReadFromThePluginConfFirstThenFeConf() { + Assertions.assertEquals("/from/plugin/conf", ConnectorConf.get( + context(Map.of(JdbcConnectorProperties.CONF_DRIVERS_DIR, "/from/plugin/conf"), + Map.of(JdbcConnectorProperties.ENV_DRIVERS_DIR, "/from/fe/conf")), + JdbcConnectorProperties.CONF_DRIVERS_DIR, + JdbcConnectorProperties.ENV_DRIVERS_DIR, null)); + + Assertions.assertEquals("/from/fe/conf", ConnectorConf.get( + context(Map.of(), Map.of(JdbcConnectorProperties.ENV_DRIVERS_DIR, "/from/fe/conf")), + JdbcConnectorProperties.CONF_DRIVERS_DIR, + JdbcConnectorProperties.ENV_DRIVERS_DIR, null)); + } + + @Test + void theConfTemplateIsNamedAfterTheProvider() { + // The engine reads .conf, so a template under any other name deploys a file nothing ever + // opens -- silently, with every setting in it ignored. Renaming getType() must break here. + String expected = new JdbcConnectorProvider().name() + ".conf.template"; + Assertions.assertNotNull(getClass().getClassLoader().getResource(expected), + "the plugin must ship " + expected + " on its classpath"); + } + + private static ConnectorContext context(Map conf, Map env) { + return new ConnectorContext() { + @Override + public String getCatalogName() { + return "test_catalog"; + } + + @Override + public long getCatalogId() { + return 1L; + } + + @Override + public Map getConnectorConfig() { + return conf; + } + + @Override + public Map getEnvironment() { + return env; + } + }; + } + private static int countOccurrences(String str, String sub) { int count = 0; int idx = 0; diff --git a/fe/fe-connector/fe-connector-metastore-paimon/src/test/java/org/apache/doris/connector/metastore/paimon/jdbc/PaimonJdbcMetaStorePropertiesTest.java b/fe/fe-connector/fe-connector-metastore-paimon/src/test/java/org/apache/doris/connector/metastore/paimon/jdbc/PaimonJdbcMetaStorePropertiesTest.java index cc6842020b4e4a..9916654c9b6139 100644 --- a/fe/fe-connector/fe-connector-metastore-paimon/src/test/java/org/apache/doris/connector/metastore/paimon/jdbc/PaimonJdbcMetaStorePropertiesTest.java +++ b/fe/fe-connector/fe-connector-metastore-paimon/src/test/java/org/apache/doris/connector/metastore/paimon/jdbc/PaimonJdbcMetaStorePropertiesTest.java @@ -74,21 +74,28 @@ public void validateChecksWarehouseThenUriThenDriverClass() { @Test public void resolveDriverUrl() { - Map env = new HashMap<>(); + // The drivers directory is now passed in rather than read from the engine environment here: + // which settings file it comes from is the calling connector's business, and this module is + // shared by connectors whose conf files differ. The resolution itself is unchanged. // already scheme-bearing -> as-is - Assertions.assertEquals("https://host/d.jar", JdbcDriverSupport.resolveDriverUrl("https://host/d.jar", env)); + Assertions.assertEquals("https://host/d.jar", + JdbcDriverSupport.resolveDriverUrl("https://host/d.jar", null, null)); // absolute path -> as-is (no driversDir prepend) - Assertions.assertEquals("/opt/drivers/d.jar", JdbcDriverSupport.resolveDriverUrl("/opt/drivers/d.jar", env)); + Assertions.assertEquals("/opt/drivers/d.jar", + JdbcDriverSupport.resolveDriverUrl("/opt/drivers/d.jar", null, null)); // bare jar with explicit drivers dir - env.put("jdbc_drivers_dir", "/custom/drivers"); - Assertions.assertEquals("file:///custom/drivers/d.jar", JdbcDriverSupport.resolveDriverUrl("d.jar", env)); + Assertions.assertEquals("file:///custom/drivers/d.jar", + JdbcDriverSupport.resolveDriverUrl("d.jar", "/custom/drivers", "/dh")); // bare jar falling back to doris_home/plugins/jdbc_drivers - Map env2 = new HashMap<>(); - env2.put("doris_home", "/dh"); - Assertions.assertEquals("file:///dh/plugins/jdbc_drivers/d.jar", JdbcDriverSupport.resolveDriverUrl("d.jar", env2)); - // empty env -> doris_home defaults to "." + Assertions.assertEquals("file:///dh/plugins/jdbc_drivers/d.jar", + JdbcDriverSupport.resolveDriverUrl("d.jar", null, "/dh")); + // a blank drivers dir falls back the same way as an absent one -- 'drivers_dir=' in a conf + // file means "not configured", not "resolve under the empty path". + Assertions.assertEquals("file:///dh/plugins/jdbc_drivers/d.jar", + JdbcDriverSupport.resolveDriverUrl("d.jar", " ", "/dh")); + // neither known -> doris_home defaults to "." Assertions.assertEquals("file://./plugins/jdbc_drivers/d.jar", - JdbcDriverSupport.resolveDriverUrl("d.jar", new HashMap<>())); + JdbcDriverSupport.resolveDriverUrl("d.jar", null, null)); } @Test diff --git a/fe/fe-connector/fe-connector-metastore-spi/src/main/java/org/apache/doris/connector/metastore/spi/JdbcDriverSupport.java b/fe/fe-connector/fe-connector-metastore-spi/src/main/java/org/apache/doris/connector/metastore/spi/JdbcDriverSupport.java index 176763295a3d3e..f1703299f3eb2a 100644 --- a/fe/fe-connector/fe-connector-metastore-spi/src/main/java/org/apache/doris/connector/metastore/spi/JdbcDriverSupport.java +++ b/fe/fe-connector/fe-connector-metastore-spi/src/main/java/org/apache/doris/connector/metastore/spi/JdbcDriverSupport.java @@ -19,8 +19,6 @@ import org.apache.commons.lang3.StringUtils; -import java.util.Map; - /** * Shared JDBC driver-url resolution. Only the PURE resolver lives here (a function of the raw * {@code driver_url} + the engine environment map). The live driver REGISTRATION @@ -36,16 +34,23 @@ private JdbcDriverSupport() { /** * Resolves a JDBC {@code driver_url} to a full, scheme-bearing URL string. A value already * carrying a scheme ({@code "://"}) is used as-is; an absolute path (starting with {@code "/"}) is - * returned unchanged; otherwise it is treated as a bare jar file name and resolved against the - * engine's configured {@code jdbc_drivers_dir} (defaulting to - * {@code $DORIS_HOME/plugins/jdbc_drivers}). Mirrors the minimal {@code JdbcResource.getFullDriverUrl} - * resolution (no file-existence / legacy old-dir / cloud-download handling), so the FE driver - * registration and the BE-bound options resolve a given {@code driver_url} identically. + * returned unchanged; otherwise it is treated as a bare jar file name and resolved against + * {@code driversDir} (defaulting to {@code $DORIS_HOME/plugins/jdbc_drivers}). Mirrors the minimal + * {@code JdbcResource.getFullDriverUrl} resolution (no file-existence / legacy old-dir / + * cloud-download handling), so the FE driver registration and the BE-bound options resolve a given + * {@code driver_url} identically. + * + *

    Both directories are passed in rather than read from the engine environment here: which + * settings file a drivers directory comes from is the calling connector's business (its own + * {@code .conf} first, then fe.conf's {@code jdbc_drivers_dir}), and this module is shared by + * connectors whose conf files differ. Same shape as + * {@code AbstractHmsMetaStoreProperties}, which likewise takes its default as a parameter. * - * @param driverUrl the raw driver_url; must be non-null and non-blank (the caller's responsibility) - * @param env the engine environment map (e.g. {@code jdbc_drivers_dir}, {@code doris_home}); never null + * @param driverUrl the raw driver_url; must be non-null and non-blank (the caller's responsibility) + * @param driversDir directory a bare jar name resolves under; blank falls back to the default below + * @param dorisHome the FE install root, used only to build that default; blank means "." */ - public static String resolveDriverUrl(String driverUrl, Map env) { + public static String resolveDriverUrl(String driverUrl, String driversDir, String dorisHome) { if (driverUrl.contains("://")) { return driverUrl; } @@ -53,11 +58,10 @@ public static String resolveDriverUrl(String driverUrl, Map env) // Absolute path, no scheme: legacy returns it as-is (no driversDir prepend). return driverUrl; } - String driversDir = env.get("jdbc_drivers_dir"); - if (StringUtils.isBlank(driversDir)) { - String dorisHome = env.getOrDefault("doris_home", "."); - driversDir = dorisHome + "/plugins/jdbc_drivers"; + String resolvedDir = driversDir; + if (StringUtils.isBlank(resolvedDir)) { + resolvedDir = (StringUtils.isBlank(dorisHome) ? "." : dorisHome) + "/plugins/jdbc_drivers"; } - return "file://" + driversDir + "/" + driverUrl; + return "file://" + resolvedDir + "/" + driverUrl; } } diff --git a/fe/fe-connector/fe-connector-paimon/src/main/assembly/plugin-zip.xml b/fe/fe-connector/fe-connector-paimon/src/main/assembly/plugin-zip.xml index 9deeb4c7889d5f..1f963509839ba9 100644 --- a/fe/fe-connector/fe-connector-paimon/src/main/assembly/plugin-zip.xml +++ b/fe/fe-connector/fe-connector-paimon/src/main/assembly/plugin-zip.xml @@ -36,6 +36,20 @@ under the License. + + + + src/main/resources/paimon.conf.template + / + + + lib diff --git a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnector.java b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnector.java index dc660eaff824ee..670ee5b006cdf0 100644 --- a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnector.java +++ b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnector.java @@ -28,6 +28,7 @@ import org.apache.doris.connector.metastore.HmsMetaStoreProperties; import org.apache.doris.connector.metastore.spi.JdbcDriverSupport; import org.apache.doris.connector.metastore.spi.MetaStoreProviders; +import org.apache.doris.connector.spi.ConnectorConf; import org.apache.doris.connector.spi.ConnectorContext; import org.apache.doris.connector.spi.ConnectorStorageContext; import org.apache.doris.filesystem.properties.StorageProperties; @@ -50,7 +51,6 @@ import java.net.MalformedURLException; import java.net.URL; import java.net.URLClassLoader; -import java.util.Collections; import java.util.EnumSet; import java.util.HashMap; import java.util.List; @@ -422,8 +422,10 @@ private Catalog createCatalog() { MetaStoreProviders.bind(properties, storageHadoopConfig); HiveConf hc = PaimonCatalogFactory.assembleHiveConf( PaimonCatalogFactory.firstNonBlank(properties, "hive.conf.resources"), - hms.toHiveConfOverrides(context.getEnvironment() - .getOrDefault("hive_metastore_client_timeout_second", "10"))); + hms.toHiveConfOverrides(ConnectorConf.get(context, + PaimonConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND))); return createCatalogFromContext(CatalogContext.create(options, hc), flavor, "Failed to create Paimon catalog with HMS metastore"); } @@ -532,8 +534,9 @@ private void maybeRegisterJdbcDriver() { * allow-list (a pre-existing fe-core gap shared by all plugin connectors — see deviations-log). */ private String resolveFullDriverUrl(String driverUrl) { - Map env = context != null ? context.getEnvironment() : Collections.emptyMap(); - return JdbcDriverSupport.resolveDriverUrl(driverUrl, env); + return JdbcDriverSupport.resolveDriverUrl(driverUrl, + PaimonConnectorProperties.configuredDriversDir(context), + PaimonConnectorProperties.configuredDorisHome(context)); } private void registerJdbcDriver(String driverUrl, String driverClassName) { diff --git a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnectorProperties.java b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnectorProperties.java index ae3fe94a25c77d..e8ac1b4257b502 100644 --- a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnectorProperties.java +++ b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonConnectorProperties.java @@ -17,11 +17,13 @@ package org.apache.doris.connector.paimon; +import org.apache.doris.connector.spi.ConnectorConf; +import org.apache.doris.connector.spi.ConnectorContext; + /** - * Property key constants for Paimon connector configuration. - * - *

    Pure static-constant holder (no logic), mirroring the role of - * {@code MCConnectorProperties}. Where a Doris-facing property accepts multiple + * Property key constants for Paimon connector configuration, plus the accessors that read this + * connector's deployment-level settings (same shape as {@code HiveConnectorProperties.getInt} / + * {@code JdbcConnectorProperties.getInt}). Where a Doris-facing property accepts multiple * aliases (matching the legacy fe-core {@code @ConnectorProperty(names = {...})} * declarations), the aliases are exposed as a {@code String[]} in alias-priority * order so {@link PaimonCatalogFactory} can resolve them with @@ -29,6 +31,48 @@ */ public final class PaimonConnectorProperties { + // -- Deployment-level settings, read from this plugin's own paimon.conf (named after + // ConnectorProvider.name()). Each falls back to the ENV_ name below it, which is the fe.conf key it + // used to live under and still works. + // + // Both are shared with other connectors at the fe.conf end -- one jdbc_drivers_dir and one + // hive_metastore_client_timeout_second serve jdbc, iceberg and paimon. A plugin conf cannot express + // that, so a deployment that moves to these files sets the value in each plugin's own conf. That is + // the accepted cost of a per-plugin file; the fe.conf keys stay as the shared fallback. -- + public static final String CONF_DRIVERS_DIR = "drivers_dir"; + public static final String CONF_METASTORE_CLIENT_TIMEOUT_SECOND = "metastore_client_timeout_second"; + + /** The fe.conf name of {@link #CONF_DRIVERS_DIR}, forwarded through the engine environment. */ + public static final String ENV_JDBC_DRIVERS_DIR = "jdbc_drivers_dir"; + /** The fe.conf name of {@link #CONF_METASTORE_CLIENT_TIMEOUT_SECOND}. */ + public static final String ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND = + "hive_metastore_client_timeout_second"; + /** Engine-wide, not this connector's: the FE install root. Stays in the engine environment. */ + public static final String ENV_DORIS_HOME = "doris_home"; + /** Legacy default when neither channel names a metastore client timeout. */ + public static final String DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND = "10"; + + /** + * The directory a bare driver jar name resolves under, from this plugin's own {@code paimon.conf} + * or fe.conf's {@code jdbc_drivers_dir}. Null when neither names one — {@code JdbcDriverSupport} + * then falls back to {@code /plugins/jdbc_drivers}, as before. + * + *

    Shared by the two call sites (FE driver registration in {@code PaimonConnector} and the + * BE-bound scan options in {@code PaimonScanPlanProvider}) so they cannot resolve a given + * {@code driver_url} differently — which is the same reason both delegate to + * {@code JdbcDriverSupport}. A null context is a direct-construction unit test, which has neither + * channel. + */ + public static String configuredDriversDir(ConnectorContext context) { + return context == null ? null + : ConnectorConf.get(context, CONF_DRIVERS_DIR, ENV_JDBC_DRIVERS_DIR, null); + } + + /** The FE install root. Engine-wide rather than this connector's, so it stays in the environment. */ + public static String configuredDorisHome(ConnectorContext context) { + return context == null ? null : context.getEnvironment().get(ENV_DORIS_HOME); + } + /** Paimon catalog backend type: filesystem, hms, rest, jdbc. */ public static final String PAIMON_CATALOG_TYPE = "paimon.catalog.type"; diff --git a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java index e24c70723ebe2f..c0a46f78a42290 100644 --- a/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java +++ b/fe/fe-connector/fe-connector-paimon/src/main/java/org/apache/doris/connector/paimon/PaimonScanPlanProvider.java @@ -1642,8 +1642,9 @@ Map getBackendPaimonOptions() { String driverUrl = PaimonCatalogFactory.firstNonBlank( properties, PaimonConnectorProperties.JDBC_DRIVER_URL); if (driverUrl != null) { - Map env = context != null ? context.getEnvironment() : Collections.emptyMap(); - options.put("jdbc.driver_url", JdbcDriverSupport.resolveDriverUrl(driverUrl, env)); + options.put("jdbc.driver_url", JdbcDriverSupport.resolveDriverUrl(driverUrl, + PaimonConnectorProperties.configuredDriversDir(context), + PaimonConnectorProperties.configuredDorisHome(context))); String driverClass = PaimonCatalogFactory.firstNonBlank( properties, PaimonConnectorProperties.JDBC_DRIVER_CLASS); if (driverClass != null) { diff --git a/fe/fe-connector/fe-connector-paimon/src/main/resources/paimon.conf.template b/fe/fe-connector/fe-connector-paimon/src/main/resources/paimon.conf.template new file mode 100644 index 00000000000000..f71c1e28fdc6b1 --- /dev/null +++ b/fe/fe-connector/fe-connector-paimon/src/main/resources/paimon.conf.template @@ -0,0 +1,25 @@ +# Paimon connector plugin configuration. +# +# build.sh seeds paimon.conf from this file on first deploy and never overwrites it, so an upgrade +# that unzips a new plugin build over this directory keeps whatever you configured here. +# This file must exist on EVERY FE node -- it is not replicated through Doris metadata. +# Changes take effect after an FE restart. +# +# Every setting below is optional. Left commented out, each falls back to the fe.conf key named +# after it, and then to the built-in default -- so an existing deployment needs no change here. +# +# NOTE: both settings below are shared with other connectors at the fe.conf end -- one +# jdbc_drivers_dir and one hive_metastore_client_timeout_second serve jdbc, iceberg and paimon. +# A per-plugin conf cannot express that, so if you move them here you must also set them in +# jdbc.conf and iceberg.conf. Leaving them commented out keeps the single shared fe.conf value. + +# Directory a bare driver jar name in paimon.jdbc.driver_url (or jdbc.driver_url) resolves under +# (JDBC catalog only). The same value is sent to BE with the scan options, so FE and BE resolve a +# given driver_url identically. +# Falls back to fe.conf's jdbc_drivers_dir, whose default is /plugins/jdbc_drivers. +# drivers_dir=/opt/doris/plugins/jdbc_drivers + +# Socket timeout, in seconds, for this catalog's Hive metastore client (HMS catalog only), used +# unless the catalog overrides it. Falls back to fe.conf's hive_metastore_client_timeout_second, +# whose default is 10. +# metastore_client_timeout_second=10 diff --git a/fe/fe-connector/fe-connector-paimon/src/test/java/org/apache/doris/connector/paimon/PaimonConnectorConfTest.java b/fe/fe-connector/fe-connector-paimon/src/test/java/org/apache/doris/connector/paimon/PaimonConnectorConfTest.java new file mode 100644 index 00000000000000..01af5258223bc6 --- /dev/null +++ b/fe/fe-connector/fe-connector-paimon/src/test/java/org/apache/doris/connector/paimon/PaimonConnectorConfTest.java @@ -0,0 +1,127 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.paimon; + +import org.apache.doris.connector.spi.ConnectorConf; +import org.apache.doris.connector.spi.ConnectorContext; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Collections; +import java.util.HashMap; +import java.util.Map; + +/** + * How this connector's two deployment-level settings are resolved: its own {@code paimon.conf} first, + * then the fe.conf key each used to live under. + * + *

    Asserted rather than trusted, because both failure directions are silent. Reading fe.conf first + * would make an administrator's edit to paimon.conf do nothing. Dropping the fe.conf fallback would + * change an untouched deployment's metastore timeout and drivers directory on upgrade, with nothing + * in either file to show why. + */ +public class PaimonConnectorConfTest { + + @Test + public void driversDirPrefersThePluginConfThenFeConf() { + Assertions.assertEquals("/from/plugin/conf", PaimonConnectorProperties.configuredDriversDir( + context(Collections.singletonMap(PaimonConnectorProperties.CONF_DRIVERS_DIR, + "/from/plugin/conf"), + Collections.singletonMap(PaimonConnectorProperties.ENV_JDBC_DRIVERS_DIR, + "/from/fe/conf")))); + + Assertions.assertEquals("/from/fe/conf", PaimonConnectorProperties.configuredDriversDir( + context(Collections.emptyMap(), + Collections.singletonMap(PaimonConnectorProperties.ENV_JDBC_DRIVERS_DIR, + "/from/fe/conf")))); + + Assertions.assertNull(PaimonConnectorProperties.configuredDriversDir( + context(Collections.emptyMap(), Collections.emptyMap()))); + } + + @Test + public void nullContextResolvesToNothingRatherThanThrowing() { + // Both accessors are reached from direct-construction unit tests that pass no context at all + // (PaimonConnector.resolveFullDriverUrl / PaimonScanPlanProvider both null-check today). + Assertions.assertNull(PaimonConnectorProperties.configuredDriversDir(null)); + Assertions.assertNull(PaimonConnectorProperties.configuredDorisHome(null)); + } + + @Test + public void metastoreTimeoutPrefersThePluginConfThenFeConfThenTen() { + Assertions.assertEquals("30", ConnectorConf.get( + context(Collections.singletonMap( + PaimonConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, "30"), + Collections.singletonMap( + PaimonConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, "20")), + PaimonConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND)); + + Assertions.assertEquals("20", ConnectorConf.get( + context(Collections.emptyMap(), Collections.singletonMap( + PaimonConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, "20")), + PaimonConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND)); + + // The literal the call site used before this channel existed; keeping it is what makes a + // deployment with neither file behave exactly as it did. + Assertions.assertEquals("10", ConnectorConf.get( + context(Collections.emptyMap(), Collections.emptyMap()), + PaimonConnectorProperties.CONF_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.ENV_HIVE_METASTORE_CLIENT_TIMEOUT_SECOND, + PaimonConnectorProperties.DEFAULT_METASTORE_CLIENT_TIMEOUT_SECOND)); + } + + @Test + public void theConfTemplateIsNamedAfterTheProvider() { + // The engine reads .conf, so a template under any other name deploys a file nothing ever + // opens -- silently, with every setting in it ignored. Renaming getType() must break here. + String expected = new PaimonConnectorProvider().name() + ".conf.template"; + Assertions.assertNotNull(getClass().getClassLoader().getResource(expected), + "the plugin must ship " + expected + " on its classpath"); + } + + private static ConnectorContext context(Map conf, Map env) { + Map confCopy = new HashMap<>(conf); + Map envCopy = new HashMap<>(env); + return new ConnectorContext() { + @Override + public String getCatalogName() { + return "test_catalog"; + } + + @Override + public long getCatalogId() { + return 1L; + } + + @Override + public Map getConnectorConfig() { + return confCopy; + } + + @Override + public Map getEnvironment() { + return envCopy; + } + }; + } +} diff --git a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorConf.java b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorConf.java new file mode 100644 index 00000000000000..4533f98ade3e29 --- /dev/null +++ b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorConf.java @@ -0,0 +1,77 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.spi; + +/** + * Reads one deployment-level setting for a connector, from the plugin's own {@code .conf} first + * and from fe.conf second. + * + *

    Two channels exist because the plugin conf file is the newer one. Settings that predate it are + * declared as {@code @ConfField}s in fe.conf and forwarded through + * {@link ConnectorContext#getEnvironment()}; those {@code @ConfField}s are kept so that an existing + * deployment keeps working untouched after an upgrade. The precedence below is what lets both be true + * at once, and it lives here — in one place — so that the connectors reading these settings cannot + * drift apart on it. + * + *

    A setting introduced from now on has no fe.conf half: pass {@code null} for + * {@code legacyEnvKey} and the engine needs no change to carry it. + */ +public final class ConnectorConf { + + private ConnectorConf() { + } + + /** + * Resolves {@code key} as: the plugin's {@code .conf}, then fe.conf, then {@code defaultValue}. + * + *

    A value that is absent or blank is "not set" at each step. Blank counts as unset + * because an operator who writes {@code key=} in a conf file means "I have not configured this", + * not "configure this to the empty string" — and reading it the other way would let an empty line + * mask the fe.conf value that is actually in effect. + * + *

    Values are returned as written (the conf file's were trimmed when the file was parsed; fe.conf's + * are passed through untouched, so a connector sees exactly the string it saw before this channel + * existed). + * + * @param context the connector's context + * @param key the key in {@code .conf}. Not prefixed with the connector's name — + * the file name already namespaces it + * @param legacyEnvKey the same setting's fe.conf name as forwarded through + * {@link ConnectorContext#getEnvironment()}, or null for a setting that never + * had one + * @param defaultValue returned when neither channel has the setting; may be null + */ + public static String get(ConnectorContext context, String key, String legacyEnvKey, + String defaultValue) { + String fromConf = context.getConnectorConfig().get(key); + if (isSet(fromConf)) { + return fromConf; + } + if (legacyEnvKey != null) { + String fromEnv = context.getEnvironment().get(legacyEnvKey); + if (isSet(fromEnv)) { + return fromEnv; + } + } + return defaultValue; + } + + private static boolean isSet(String value) { + return value != null && !value.trim().isEmpty(); + } +} diff --git a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorConfFile.java b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorConfFile.java new file mode 100644 index 00000000000000..9df36e7b44d3ec --- /dev/null +++ b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorConfFile.java @@ -0,0 +1,95 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.spi; + +import java.io.IOException; +import java.io.InputStream; +import java.io.InputStreamReader; +import java.io.Reader; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Collections; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Objects; +import java.util.Properties; + +/** + * Reads a connector plugin's own configuration file, {@code /.conf}. + * + *

    Engine side only. A connector never calls this: the engine loads the file once at plugin + * load and serves the parsed map through {@link ConnectorContext#getConnectorConfig()}, which is what + * connectors read (via {@link ConnectorConf}). Keeping the parsing here means one implementation of + * "find it, parse it, decide what an absent file means" instead of one per plugin. + * + *

    No path validation is needed or wanted here. The file name is {@code name + ".conf"} where + * {@code name} is a plugin name that {@code PluginNames.validate} has already confined to + * {@code [a-zA-Z0-9._-]}. No separator can appear in it, so no traversal is expressible. A redundant + * check added later would only suggest the constraint lives here, when it lives there. + */ +public final class ConnectorConfFile { + + /** The suffix that makes a plugin directory entry a configuration file. */ + public static final String SUFFIX = ".conf"; + + private ConnectorConfFile() { + } + + /** The configuration file name for a plugin called {@code name}. For logs and error messages. */ + public static String fileName(String name) { + return name + SUFFIX; + } + + /** + * Parses {@code /.conf} in {@link Properties} text format, the same shape as fe.conf. + * + *

    An absent file is not an error and yields an empty map: a connector whose settings all + * have defaults (or a fe.conf fallback) never needs the file, and requiring one would mean every + * deployment carries a file full of commented-out lines. + * + *

    Values are trimmed. A key present with an empty value is kept rather than dropped, so the + * map reflects the file as written; {@link ConnectorConf#get} is where "written but blank" is decided + * to mean "not set". + * + * @return an immutable map, never null + * @throws IOException if the file exists but cannot be read or parsed + */ + public static Map load(Path pluginDir, String name) throws IOException { + Objects.requireNonNull(pluginDir, "pluginDir"); + Objects.requireNonNull(name, "name"); + Path file = pluginDir.resolve(fileName(name)); + if (!Files.isRegularFile(file)) { + return Collections.emptyMap(); + } + Properties properties = new Properties(); + try (InputStream in = Files.newInputStream(file); + Reader reader = new InputStreamReader(in, StandardCharsets.UTF_8)) { + properties.load(reader); + } catch (IllegalArgumentException e) { + // Properties.load throws this (unchecked) on a malformed \\uXXXX escape. Rethrown as + // IOException so the caller's single "the file is unusable" branch covers every way it can be. + throw new IOException("malformed content in " + file, e); + } + Map parsed = new LinkedHashMap<>(); + for (String key : properties.stringPropertyNames()) { + parsed.put(key, properties.getProperty(key).trim()); + } + return Collections.unmodifiableMap(parsed); + } +} diff --git a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorContext.java b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorContext.java index eb20d9a44e6d5a..590801e10aee38 100644 --- a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorContext.java +++ b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorContext.java @@ -55,6 +55,33 @@ default Map getEnvironment() { return Collections.emptyMap(); } + /** + * The contents of {@code .conf} in this connector's own plugin directory, keys and values + * verbatim, immutable. {@code } is this connector's {@link ConnectorProvider#name()}. + * + *

    This is a connector's deployment-level configuration channel: one per FE process, + * maintained by an administrator in the plugin directory, and not settable by a user in + * {@code CREATE CATALOG}. A value that varies per catalog belongs in the property map handed to + * {@code ConnectorProvider.create}; a value that varies per query belongs in + * {@code ConnectorSession.getSessionProperties()}. + * + *

    Unlike {@link #getEnvironment()}, adding a key here costs the engine nothing: the file is named + * after the plugin and parsed generically, so no key name of yours ever appears in {@code fe-core}. + * Read it through {@link ConnectorConf#get}, which layers this map over {@code getEnvironment()} for + * keys that predate this channel. + * + *

    Never null. Returns an empty map when: the file does not exist; the file could not be read (the + * engine has already logged an ERROR); or the connector was not loaded from a plugin directory at all + * (a classpath built-in, or a provider registered by a test). + * + *

    Engine side: {@code ConnectorPluginManager.loadPlugins} reads the file once, right after the + * provider is admitted, and {@code ConnectorPluginManager.createConnector} attaches it to the context + * it hands {@code ConnectorProvider.create}. Editing the file needs an FE restart, same as fe.conf. + */ + default Map getConnectorConfig() { + return Collections.emptyMap(); + } + /** * Returns the HTTP security hook for SSRF protection. * Connectors making outbound HTTP requests should call this hook diff --git a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorProvider.java b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorProvider.java index 33368ff2fff9a5..80f8cf815d7077 100644 --- a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorProvider.java +++ b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorProvider.java @@ -177,6 +177,21 @@ default String displayEngineName() { return getType(); } + /** + * This plugin's identity to the engine. Defaults to {@link #getType()}; two loaded plugins may not + * share one ({@code ConnectorPluginManager.loadPlugins} skips the second). + * + *

    It is also the name of this connector's own configuration file: the engine reads + * {@code /.conf} and serves it back through + * {@link ConnectorContext#getConnectorConfig()}. So a connector that ships a conf template must name + * it {@code .conf.template}, and changing this method renames that file — the plugin + * directory name has no say in it, and need not match ({@code hive/hms.conf} is a shipped example). + * Guard it with a test that the template resource exists under {@code name() + ".conf.template"}. + * + *

    The engine builds a file name out of this string, which is safe because + * {@code PluginNames.validate} has already confined it to {@code [a-zA-Z0-9._-]} — no separator can + * reach a path. Do not re-validate it. + */ @Override default String name() { return getType(); diff --git a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ForwardingConnectorContext.java b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ForwardingConnectorContext.java index a74bf71a41f22c..b263fc16d82f3b 100644 --- a/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ForwardingConnectorContext.java +++ b/fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ForwardingConnectorContext.java @@ -83,6 +83,11 @@ public Map getEnvironment() { return delegate.getEnvironment(); } + @Override + public Map getConnectorConfig() { + return delegate.getConnectorConfig(); + } + @Override public ConnectorHttpSecurityHook getHttpSecurityHook() { return delegate.getHttpSecurityHook(); diff --git a/fe/fe-connector/fe-connector-spi/src/test/java/org/apache/doris/connector/spi/ConnectorConfFileTest.java b/fe/fe-connector/fe-connector-spi/src/test/java/org/apache/doris/connector/spi/ConnectorConfFileTest.java new file mode 100644 index 00000000000000..dd2c68a24ff27c --- /dev/null +++ b/fe/fe-connector/fe-connector-spi/src/test/java/org/apache/doris/connector/spi/ConnectorConfFileTest.java @@ -0,0 +1,112 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.spi; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Map; + +public class ConnectorConfFileTest { + + @TempDir + private Path pluginDir; + + private Map write(String content) throws IOException { + Files.write(pluginDir.resolve("demo.conf"), content.getBytes(StandardCharsets.UTF_8)); + return ConnectorConfFile.load(pluginDir, "demo"); + } + + @Test + public void fileName_isNamePlusSuffix() { + // The plugin's name IS the file name; anything else and the engine looks for a file no plugin ships. + Assertions.assertEquals("hms.conf", ConnectorConfFile.fileName("hms")); + Assertions.assertEquals("trino-connector.conf", ConnectorConfFile.fileName("trino-connector")); + } + + @Test + public void missingFile_isEmptyMapNotAnError() throws IOException { + // Deliberate: a connector whose settings all have defaults or a fe.conf fallback ships no conf at + // all. Making this an error would force every deployment to carry a file of commented-out lines. + Assertions.assertTrue(ConnectorConfFile.load(pluginDir, "demo").isEmpty()); + } + + @Test + public void directoryNamedLikeTheConfFile_isEmptyMapNotAnError() throws IOException { + // isRegularFile, not exists: a directory that happens to be called demo.conf must not blow up + // plugin loading -- it is not a conf file, so the connector simply has none. + Files.createDirectory(pluginDir.resolve("demo.conf")); + Assertions.assertTrue(ConnectorConfFile.load(pluginDir, "demo").isEmpty()); + } + + @Test + public void parsesCommentsBlankLinesAndTrimsValues() throws IOException { + Map conf = write("# a comment\n" + + "\n" + + "drivers_dir = /opt/drivers \n" + + "! another comment style\n" + + "timeout=30\n"); + Assertions.assertEquals(2, conf.size(), conf.toString()); + // Trimmed, because an operator lining up '=' signs is not configuring a path with spaces in it. + Assertions.assertEquals("/opt/drivers", conf.get("drivers_dir")); + Assertions.assertEquals("30", conf.get("timeout")); + } + + @Test + public void blankValueIsKeptSoTheMapReflectsTheFile() throws IOException { + // The loader reports the file as written; deciding that "written but blank" means "not set" is + // ConnectorConf.get's job. Dropping the key here would erase that distinction before anyone + // could act on it -- and would make a future information_schema view lie about the file. + Map conf = write("drivers_dir=\nother= \n"); + Assertions.assertTrue(conf.containsKey("drivers_dir")); + Assertions.assertEquals("", conf.get("drivers_dir")); + Assertions.assertEquals("", conf.get("other")); + } + + @Test + public void readsUtf8NotIso8859() throws IOException { + // Properties.load(InputStream) would decode as ISO-8859-1 and mangle this; the reader overload + // with an explicit UTF-8 charset is what keeps a non-ASCII path usable. + Map conf = write("warehouse=/数据/仓库\n"); + Assertions.assertEquals("/数据/仓库", conf.get("warehouse")); + } + + @Test + public void malformedContent_throwsIoExceptionNamingTheFile() throws IOException { + // Properties.load throws an unchecked IllegalArgumentException on a bad \\uXXXX escape. It is + // rethrown as IOException so the engine's single "this file is unusable" catch covers every way + // the file can be bad -- an unchecked escape would instead abort plugin loading entirely. + Files.write(pluginDir.resolve("demo.conf"), "k=\\uZZZZ\n".getBytes(StandardCharsets.UTF_8)); + IOException e = Assertions.assertThrows(IOException.class, + () -> ConnectorConfFile.load(pluginDir, "demo")); + Assertions.assertTrue(e.getMessage().contains("demo.conf"), e.getMessage()); + } + + @Test + public void returnedMapIsImmutable() throws IOException { + // It is handed to plugin code through getConnectorConfig(); a plugin must not be able to edit + // what another catalog of the same type will read next. + Map conf = write("k=v\n"); + Assertions.assertThrows(UnsupportedOperationException.class, () -> conf.put("k2", "v2")); + } +} diff --git a/fe/fe-connector/fe-connector-spi/src/test/java/org/apache/doris/connector/spi/ConnectorConfTest.java b/fe/fe-connector/fe-connector-spi/src/test/java/org/apache/doris/connector/spi/ConnectorConfTest.java new file mode 100644 index 00000000000000..5bfe9791e0fb72 --- /dev/null +++ b/fe/fe-connector/fe-connector-spi/src/test/java/org/apache/doris/connector/spi/ConnectorConfTest.java @@ -0,0 +1,140 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector.spi; + +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +import java.util.Collections; +import java.util.Map; + +public class ConnectorConfTest { + + private static ConnectorContext context(Map conf, Map env) { + return new ConnectorContext() { + @Override + public String getCatalogName() { + return "test_catalog"; + } + + @Override + public long getCatalogId() { + return 1L; + } + + @Override + public Map getConnectorConfig() { + return conf; + } + + @Override + public Map getEnvironment() { + return env; + } + }; + } + + @Test + public void pluginConfWinsOverFeConf() { + // The whole point of the channel: an administrator who sets the key in the plugin's conf gets + // that value, whatever fe.conf still says. Reverse this precedence and migrating a deployment + // to the new channel silently does nothing. + ConnectorContext ctx = context(Collections.singletonMap("drivers_dir", "/from/plugin/conf"), + Collections.singletonMap("jdbc_drivers_dir", "/from/fe/conf")); + Assertions.assertEquals("/from/plugin/conf", + ConnectorConf.get(ctx, "drivers_dir", "jdbc_drivers_dir", "/default")); + } + + @Test + public void fallsBackToFeConfWhenThePluginConfHasNoSuchKey() { + // This is what makes the migration non-breaking: an untouched deployment ships no plugin conf, + // so every one of these settings must still resolve to the fe.conf value it resolved to before. + ConnectorContext ctx = context(Collections.emptyMap(), + Collections.singletonMap("jdbc_drivers_dir", "/from/fe/conf")); + Assertions.assertEquals("/from/fe/conf", + ConnectorConf.get(ctx, "drivers_dir", "jdbc_drivers_dir", "/default")); + } + + @Test + public void blankInThePluginConfFallsBackRatherThanMaskingFeConf() { + // 'drivers_dir=' in a conf file reads as "I have not configured this", not "configure it to the + // empty string". Treating it as a set value would let one stray line hide the value actually in + // effect, and the operator would have no way to tell from either file which one won. + ConnectorContext ctx = context(Collections.singletonMap("drivers_dir", " "), + Collections.singletonMap("jdbc_drivers_dir", "/from/fe/conf")); + Assertions.assertEquals("/from/fe/conf", + ConnectorConf.get(ctx, "drivers_dir", "jdbc_drivers_dir", "/default")); + } + + @Test + public void blankInFeConfFallsBackToTheDefault() { + ConnectorContext ctx = context(Collections.emptyMap(), + Collections.singletonMap("jdbc_drivers_dir", "")); + Assertions.assertEquals("/default", + ConnectorConf.get(ctx, "drivers_dir", "jdbc_drivers_dir", "/default")); + } + + @Test + public void defaultWhenNeitherChannelHasIt() { + ConnectorContext ctx = context(Collections.emptyMap(), Collections.emptyMap()); + Assertions.assertEquals("/default", + ConnectorConf.get(ctx, "drivers_dir", "jdbc_drivers_dir", "/default")); + Assertions.assertNull(ConnectorConf.get(ctx, "drivers_dir", "jdbc_drivers_dir", null)); + } + + @Test + public void nullLegacyKeyNeverReadsTheEnvironment() { + // A setting introduced after this channel has no fe.conf half. It must not accidentally pick up + // an unrelated engine environment entry that happens to share its (unprefixed) name -- 'doris_home' + // and 'doris_version' live in that same map. + ConnectorContext ctx = context(Collections.emptyMap(), + Collections.singletonMap("drivers_dir", "/from/env")); + Assertions.assertEquals("/default", + ConnectorConf.get(ctx, "drivers_dir", null, "/default")); + } + + @Test + public void feConfValueIsReturnedVerbatim() { + // Byte-identical to what the connector read before this channel existed: the fallback path must + // not start trimming values that fe.conf delivered untrimmed, or a migrated connector's behavior + // would differ from the one it replaced in a way no test of the new channel would catch. + ConnectorContext ctx = context(Collections.emptyMap(), + Collections.singletonMap("hive_default_file_format", " orc ")); + Assertions.assertEquals(" orc ", + ConnectorConf.get(ctx, "default_file_format", "hive_default_file_format", "parquet")); + } + + @Test + public void worksOnAContextThatImplementsNeitherGetter() { + // Direct-construction unit tests and classpath built-ins get the interface defaults (two empty + // maps). Reading a setting must degrade to the default, not NPE. + ConnectorContext bare = new ConnectorContext() { + @Override + public String getCatalogName() { + return "test_catalog"; + } + + @Override + public long getCatalogId() { + return 1L; + } + }; + Assertions.assertEquals("/default", + ConnectorConf.get(bare, "drivers_dir", "jdbc_drivers_dir", "/default")); + } +} diff --git a/fe/fe-connector/fe-connector-spi/src/test/resources/connector-plugin-surface.txt b/fe/fe-connector/fe-connector-spi/src/test/resources/connector-plugin-surface.txt index 10e17766d0918a..287fbe3cbf151c 100644 --- a/fe/fe-connector/fe-connector-spi/src/test/resources/connector-plugin-surface.txt +++ b/fe/fe-connector/fe-connector-spi/src/test/resources/connector-plugin-surface.txt @@ -23,6 +23,7 @@ org.apache.doris.connector.spi.ConnectorContext#createSiblingConnector(java.lang org.apache.doris.connector.spi.ConnectorContext#executeAuthenticated(java.util.concurrent.Callable):java.lang.Object org.apache.doris.connector.spi.ConnectorContext#getCatalogId():long org.apache.doris.connector.spi.ConnectorContext#getCatalogName():java.lang.String +org.apache.doris.connector.spi.ConnectorContext#getConnectorConfig():java.util.Map org.apache.doris.connector.spi.ConnectorContext#getEnvironment():java.util.Map org.apache.doris.connector.spi.ConnectorContext#getHttpSecurityHook():org.apache.doris.connector.api.ConnectorHttpSecurityHook org.apache.doris.connector.spi.ConnectorContext#getStorageContext():org.apache.doris.connector.spi.ConnectorStorageContext diff --git a/fe/fe-connector/fe-connector-trino/src/main/assembly/plugin-zip.xml b/fe/fe-connector/fe-connector-trino/src/main/assembly/plugin-zip.xml index be333bbb7e616f..88edc785e8cb8c 100644 --- a/fe/fe-connector/fe-connector-trino/src/main/assembly/plugin-zip.xml +++ b/fe/fe-connector/fe-connector-trino/src/main/assembly/plugin-zip.xml @@ -42,6 +42,17 @@ under the License. ${project.build.directory}/${project.build.finalName}.jar / + + + src/main/resources/trino-connector.conf.template + / + diff --git a/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoBootstrap.java b/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoBootstrap.java index 0329f8423199fa..42b0f171969409 100644 --- a/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoBootstrap.java +++ b/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoBootstrap.java @@ -321,17 +321,10 @@ private static void configureJulLogging() { /** * Resolves the Trino plugin directory. * - *

    This plugin runs in an isolated classloader and cannot read FE {@code Config} - * (it would see its own bundled copy holding default values). The FE config - * {@code trino_connector_plugin_dir} is therefore passed in through the engine - * environment map (see {@code DefaultConnectorContext}), mirroring how the JDBC - * connector receives {@code jdbc_drivers_dir}. - * *

    Resolution order: *

      *
    1. the per-catalog {@code trino.plugin.dir} property, when set;
      2. - *
    2. otherwise the FE config {@code trino_connector_plugin_dir} from the environment, - * used verbatim (it defaults to {@code DORIS_HOME/plugins/trino_plugins}).
      3. + *
    3. otherwise {@code configuredDir}, used verbatim.
      4. *
    * *

    Nothing else is consulted: the dir the config names is the dir the plugins are loaded @@ -339,25 +332,27 @@ private static void configureJulLogging() { * {@code DORIS_HOME/plugins/connectors} when the config was left at its default, which forced * this class to duplicate the default as a literal just to tell "user set it" from "untouched". * That compatibility path was dropped deliberately — a deployment whose plugins still sit in a - * legacy dir must move them or point {@code trino_connector_plugin_dir} at them. + * legacy dir must move them or point the config at them. * - * @param properties catalog properties (unstripped, may carry {@code trino.plugin.dir}) - * @param environment engine environment from {@code ConnectorContext.getEnvironment()} + * @param properties catalog properties (unstripped, may carry {@code trino.plugin.dir}) + * @param configuredDir the deployment-level setting, already resolved by the caller from + * {@code plugin_dir} in the plugin's own conf or {@code trino_connector_plugin_dir} + * in fe.conf; null or empty means the engine delivered neither */ - public static String resolvePluginDir(Map properties, Map environment) { + public static String resolvePluginDir(Map properties, String configuredDir) { String explicitDir = properties.get("trino.plugin.dir"); if (explicitDir != null && !explicitDir.isEmpty()) { return explicitDir; } - String configuredDir = environment.get("trino_connector_plugin_dir"); if (configuredDir == null || configuredDir.isEmpty()) { - // DefaultConnectorContext always passes the FE config, which always holds a value. Absent - // means the engine failed to deliver it; guessing a dir here would surface as "catalog - // creates fine but every query fails", so fail where the cause is still visible. + // fe.conf always holds a value for this, and the engine always forwards it, so absent means + // the engine failed to deliver it. Guessing a dir here would surface as "catalog creates fine + // but every query fails", so fail where the cause is still visible. throw new IllegalStateException( - "trino_connector_plugin_dir was not delivered through the engine environment; " - + "cannot resolve the Trino plugin dir"); + "neither '" + TrinoConnectorProvider.CONF_PLUGIN_DIR + "' in " + + TrinoConnectorProvider.TYPE + ".conf nor trino_connector_plugin_dir in " + + "fe.conf was delivered; cannot resolve the Trino plugin dir"); } return configuredDir; } diff --git a/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoConnectorProvider.java b/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoConnectorProvider.java index f685261c23b231..f7e207f0b2d296 100644 --- a/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoConnectorProvider.java +++ b/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoConnectorProvider.java @@ -31,9 +31,26 @@ public class TrinoConnectorProvider implements ConnectorProvider { static final String TRINO_CONNECTOR_NAME = "trino.connector.name"; + /** + * This connector's type, and therefore its {@code name()} — which is what the engine names its + * conf file after, so the plugin must ship {@code trino-connector.conf.template}. Note that this is + * NOT the plugin directory name ({@code plugins/connector/trino}); the directory is the deployer's + * choice, the conf file name is this string. + */ + public static final String TYPE = "trino-connector"; + + /** + * Directory holding the Trino plugins this connector loads, in {@code trino-connector.conf}. + * Falls back to fe.conf's {@code trino_connector_plugin_dir}, which is where it used to live. + */ + public static final String CONF_PLUGIN_DIR = "plugin_dir"; + + /** The fe.conf name of {@link #CONF_PLUGIN_DIR}, forwarded through the engine environment. */ + public static final String ENV_PLUGIN_DIR = "trino_connector_plugin_dir"; + @Override public String getType() { - return "trino-connector"; + return TYPE; } @Override diff --git a/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoDorisConnector.java b/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoDorisConnector.java index 64f31fd8f68196..bd877e04d4a200 100644 --- a/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoDorisConnector.java +++ b/fe/fe-connector/fe-connector-trino/src/main/java/org/apache/doris/connector/trino/TrinoDorisConnector.java @@ -22,6 +22,7 @@ import org.apache.doris.connector.api.ConnectorSession; import org.apache.doris.connector.api.ConnectorValidationContext; import org.apache.doris.connector.api.scan.ConnectorScanPlanProvider; +import org.apache.doris.connector.spi.ConnectorConf; import org.apache.doris.connector.spi.ConnectorContext; import com.google.common.collect.ImmutableMap; @@ -164,9 +165,12 @@ private void doInitialize() { } // 2. Initialize Trino plugin infrastructure (singleton). - // The plugin dir comes from the FE engine environment (fe-core reads fe.conf); - // this plugin's classloader cannot see FE Config directly. - String pluginDir = TrinoBootstrap.resolvePluginDir(properties, context.getEnvironment()); + // The plugin dir is a deployment-level setting: this plugin's classloader cannot see FE Config + // directly, so it arrives either in this plugin's own trino-connector.conf or, for a deployment + // that has not moved to that file, from fe.conf through the engine environment. + String pluginDir = TrinoBootstrap.resolvePluginDir(properties, + ConnectorConf.get(context, TrinoConnectorProvider.CONF_PLUGIN_DIR, + TrinoConnectorProvider.ENV_PLUGIN_DIR, null)); TrinoBootstrap bootstrap = TrinoBootstrap.getInstance(pluginDir); // 3. Create Trino Connector + Session for this catalog diff --git a/fe/fe-connector/fe-connector-trino/src/main/resources/trino-connector.conf.template b/fe/fe-connector/fe-connector-trino/src/main/resources/trino-connector.conf.template new file mode 100644 index 00000000000000..e1d660d21fecca --- /dev/null +++ b/fe/fe-connector/fe-connector-trino/src/main/resources/trino-connector.conf.template @@ -0,0 +1,18 @@ +# Trino connector plugin configuration. +# +# build.sh seeds trino-connector.conf from this file on first deploy and never overwrites it, so an +# upgrade that unzips a new plugin build over this directory keeps whatever you configured here. +# This file must exist on EVERY FE node -- it is not replicated through Doris metadata. +# Changes take effect after an FE restart. +# +# The file name is the connector's name (ConnectorProvider.name()), NOT the directory name: this +# plugin is deployed under plugins/connector/trino/ but its settings file is trino-connector.conf. +# +# Every setting below is optional. Left commented out, each falls back to the fe.conf key named +# after it, and then to the built-in default -- so an existing deployment needs no change here. + +# Directory holding the Trino plugins this connector loads (a directory of per-plugin subdirectories). +# Falls back to fe.conf's trino_connector_plugin_dir, whose default is +# /plugins/trino_plugins. A catalog may override it per catalog with the +# trino.plugin.dir property, which wins over both. +# plugin_dir=/opt/doris/plugins/trino_plugins diff --git a/fe/fe-connector/fe-connector-trino/src/test/java/org/apache/doris/connector/trino/TrinoBootstrapTest.java b/fe/fe-connector/fe-connector-trino/src/test/java/org/apache/doris/connector/trino/TrinoBootstrapTest.java index 842b7b1b8449e8..5c56aff30460ea 100644 --- a/fe/fe-connector/fe-connector-trino/src/test/java/org/apache/doris/connector/trino/TrinoBootstrapTest.java +++ b/fe/fe-connector/fe-connector-trino/src/test/java/org/apache/doris/connector/trino/TrinoBootstrapTest.java @@ -17,6 +17,9 @@ package org.apache.doris.connector.trino; +import org.apache.doris.connector.spi.ConnectorConf; +import org.apache.doris.connector.spi.ConnectorContext; + import com.google.common.collect.ImmutableMap; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.Test; @@ -49,30 +52,19 @@ private static void installPluginIn(Path dorisHome, String subdir) throws IOExce Files.createDirectories(dorisHome.resolve(subdir).resolve("trino-hive")); } - private static Map envAtDefault(Path dorisHome) { - return ImmutableMap.of( - "doris_home", dorisHome.toString(), - "trino_connector_plugin_dir", dorisHome + "/plugins/trino_plugins"); - } - @Test public void perCatalogPropertyTakesPrecedence() { - Map env = ImmutableMap.of( - "doris_home", "/opt/doris", - "trino_connector_plugin_dir", "/should/be/ignored"); String resolved = TrinoBootstrap.resolvePluginDir( - ImmutableMap.of("trino.plugin.dir", "/custom/catalog/dir"), env); + ImmutableMap.of("trino.plugin.dir", "/custom/catalog/dir"), "/should/be/ignored"); Assertions.assertEquals("/custom/catalog/dir", resolved); } @Test - public void feConfigFromEnvironmentIsHonored() { - // Exactly what the regression environment sets in fe.conf, delivered via the - // engine environment because the plugin classloader cannot read FE Config. - Map env = ImmutableMap.of( - "doris_home", "/opt/doris", - "trino_connector_plugin_dir", "/tmp/trino_connector/connectors"); - String resolved = TrinoBootstrap.resolvePluginDir(Collections.emptyMap(), env); + public void theDeploymentLevelSettingIsHonored() { + // Exactly what the regression environment configures, delivered by the engine because the + // plugin classloader cannot read FE Config. + String resolved = TrinoBootstrap.resolvePluginDir( + Collections.emptyMap(), "/tmp/trino_connector/connectors"); Assertions.assertEquals("/tmp/trino_connector/connectors", resolved); } @@ -86,17 +78,74 @@ public void legacyPluginDirsAreNotConsultedEvenWhenTheyHoldPlugins(@TempDir Path installPluginIn(dorisHome, "connectors"); installPluginIn(dorisHome, "plugins/connectors"); - String resolved = TrinoBootstrap.resolvePluginDir(Collections.emptyMap(), envAtDefault(dorisHome)); + String resolved = TrinoBootstrap.resolvePluginDir( + Collections.emptyMap(), dorisHome + "/plugins/trino_plugins"); Assertions.assertEquals(dorisHome + "/plugins/trino_plugins", resolved); } @Test - public void missingConfigInTheEnvironmentFailsLoud() { - // DefaultConnectorContext always passes the FE config, so an absent key means the engine - // failed to deliver it. Guessing a dir would surface far away as "catalog creates fine but - // every query fails"; throwing keeps the cause at the point of breakage. + public void missingSettingFailsLoudNamingBothPlacesItCanBeSet() { + // fe.conf always holds a value for this and the engine always forwards it, so absent means the + // engine failed to deliver it. Guessing a dir would surface far away as "catalog creates fine + // but every query fails"; throwing keeps the cause at the point of breakage. The message names + // both channels because after the migration either one could be the missing half. + IllegalStateException e = Assertions.assertThrows(IllegalStateException.class, + () -> TrinoBootstrap.resolvePluginDir(Collections.emptyMap(), null)); + Assertions.assertTrue(e.getMessage().contains("trino-connector.conf"), e.getMessage()); + Assertions.assertTrue(e.getMessage().contains("trino_connector_plugin_dir"), e.getMessage()); Assertions.assertThrows(IllegalStateException.class, - () -> TrinoBootstrap.resolvePluginDir( - Collections.emptyMap(), ImmutableMap.of("doris_home", "/opt/doris"))); + () -> TrinoBootstrap.resolvePluginDir(Collections.emptyMap(), "")); + } + + @Test + public void pluginConfBeatsFeConfAndBothBeatNothing() { + // The resolution the connector performs before calling resolvePluginDir. Asserted here rather + // than trusted, because getting it backwards would make an administrator's edit to + // trino-connector.conf silently do nothing while fe.conf still holds the old value. + Assertions.assertEquals("/from/plugin/conf", ConnectorConf.get( + context(ImmutableMap.of(TrinoConnectorProvider.CONF_PLUGIN_DIR, "/from/plugin/conf"), + ImmutableMap.of(TrinoConnectorProvider.ENV_PLUGIN_DIR, "/from/fe/conf")), + TrinoConnectorProvider.CONF_PLUGIN_DIR, TrinoConnectorProvider.ENV_PLUGIN_DIR, null)); + + Assertions.assertEquals("/from/fe/conf", ConnectorConf.get( + context(ImmutableMap.of(), + ImmutableMap.of(TrinoConnectorProvider.ENV_PLUGIN_DIR, "/from/fe/conf")), + TrinoConnectorProvider.CONF_PLUGIN_DIR, TrinoConnectorProvider.ENV_PLUGIN_DIR, null)); + + Assertions.assertNull(ConnectorConf.get(context(ImmutableMap.of(), ImmutableMap.of()), + TrinoConnectorProvider.CONF_PLUGIN_DIR, TrinoConnectorProvider.ENV_PLUGIN_DIR, null)); + } + + @Test + public void theConfTemplateIsNamedAfterTheProvider() { + // The engine reads .conf, so a template under any other name deploys a file nothing ever + // opens -- silently, with every setting in it ignored. Renaming getType() must break here. + String expected = new TrinoConnectorProvider().name() + ".conf.template"; + Assertions.assertNotNull(getClass().getClassLoader().getResource(expected), + "the plugin must ship " + expected + " on its classpath"); + } + + private static ConnectorContext context(Map conf, Map env) { + return new ConnectorContext() { + @Override + public String getCatalogName() { + return "test_catalog"; + } + + @Override + public long getCatalogId() { + return 1L; + } + + @Override + public Map getConnectorConfig() { + return conf; + } + + @Override + public Map getEnvironment() { + return env; + } + }; } } diff --git a/fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorConfigContext.java b/fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorConfigContext.java new file mode 100644 index 00000000000000..259cd9801fe9e4 --- /dev/null +++ b/fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorConfigContext.java @@ -0,0 +1,47 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector; + +import org.apache.doris.connector.spi.ConnectorContext; +import org.apache.doris.connector.spi.ForwardingConnectorContext; + +import java.util.Map; +import java.util.Objects; + +/** + * A {@link ConnectorContext} carrying one plugin's own {@code .conf}. + * + *

    {@link ConnectorPluginManager#createConnector} wraps the engine context in this just before calling + * {@code ConnectorProvider.create}, because that is the first moment the engine knows which plugin the + * catalog type resolved to. Everything else forwards, so this class holds no engine behavior of its own — + * see {@link ForwardingConnectorContext} for why that base class rather than hand-written pass-throughs. + */ +final class ConnectorConfigContext extends ForwardingConnectorContext { + + private final Map conf; + + ConnectorConfigContext(ConnectorContext delegate, Map conf) { + super(delegate); + this.conf = Objects.requireNonNull(conf, "conf"); + } + + @Override + public Map getConnectorConfig() { + return conf; + } +} diff --git a/fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorPluginManager.java b/fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorPluginManager.java index cd74eeddfda21a..35e2e31cda5b0c 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorPluginManager.java +++ b/fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorPluginManager.java @@ -18,6 +18,7 @@ package org.apache.doris.connector; import org.apache.doris.connector.api.Connector; +import org.apache.doris.connector.spi.ConnectorConfFile; import org.apache.doris.connector.spi.ConnectorContext; import org.apache.doris.connector.spi.ConnectorProvider; import org.apache.doris.datasource.CatalogFactory; @@ -32,11 +33,13 @@ import org.apache.logging.log4j.LogManager; import org.apache.logging.log4j.Logger; +import java.io.IOException; import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.Collections; import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Optional; @@ -101,6 +104,18 @@ public class ConnectorPluginManager { new DirectoryPluginRuntimeManager<>(); private final ClassLoadingPolicy classLoadingPolicy = new ClassLoadingPolicy(CONNECTOR_PARENT_FIRST_PREFIXES); + /** + * Each directory-loaded provider's own {@code .conf}, read once at load and served back through + * {@link ConnectorContext#getConnectorConfig()}. + * + *

    An {@link IdentityHashMap} because the key is the provider instance and lookup must not call + * {@code equals}/{@code hashCode} on plugin code — the same rule + * {@link DirectoryPluginRuntimeManager} follows by snapshotting {@code name()} once at load and never + * re-entering the plugin on a query path. Written only during startup, read when a catalog is built, + * so a synchronized wrapper is enough. + */ + private final Map> connectorConfigs = + Collections.synchronizedMap(new IdentityHashMap<>()); /** Called at FE startup to load built-in providers from classpath. */ public void loadBuiltins() { @@ -236,6 +251,7 @@ public void loadPlugins(List pluginRoots) { // information_schema.extensions never lists a connector the routing table cannot reach. if (registerDiscovered(handle.getFactory(), false)) { PluginRegistry.getInstance().registerExternal(PLUGIN_FAMILY, handle); + loadConnectorConfig(handle); LOG.info("Loaded connector plugin: name={}, pluginDir={}, jarCount={}", handle.getPluginName(), handle.getPluginDir(), handle.getResolvedJars().size()); @@ -249,6 +265,34 @@ public void loadPlugins(List pluginRoots) { } } + /** + * Reads this plugin's own {@code .conf}, if it ships one, so that + * {@link ConnectorContext#getConnectorConfig()} can serve it later. + * + *

    Nothing here can keep the plugin from being registered. A conf file that cannot be read is + * reported and the connector proceeds without it: refusing the plugin would make the catalog type + * vanish entirely, and the only thing a user would then see is {@code CREATE CATALOG} answering + * "no provider supports type", which points nowhere near a bad file. Every setting reachable this way + * either has a default or a fe.conf fallback, so proceeding is a real degradation path, not a guess. + */ + private void loadConnectorConfig(PluginHandle handle) { + Map conf = Collections.emptyMap(); + try { + conf = ConnectorConfFile.load(handle.getPluginDir(), handle.getPluginName()); + } catch (IOException e) { + LOG.error("Failed to read connector plugin conf {} in {}; the connector starts without it " + + "and falls back to fe.conf", ConnectorConfFile.fileName(handle.getPluginName()), + handle.getPluginDir(), e); + } + connectorConfigs.put(handle.getFactory(), conf); + if (!conf.isEmpty()) { + // Key names only. A value here is administrator-supplied and may name a credential path or + // an internal host; the point of the line is to let an operator confirm the file was found. + LOG.info("Connector plugin conf loaded: name={}, file={}, keys={}", handle.getPluginName(), + ConnectorConfFile.fileName(handle.getPluginName()), conf.keySet()); + } + } + private boolean hasProviderNamed(String name) { for (ConnectorProvider p : providers) { if (name.equals(p.name())) { @@ -304,7 +348,7 @@ private Connector createConnector(String catalogType, Map proper } LOG.info("Creating connector via provider '{}' for catalogType='{}'", provider.getType(), catalogType); - return provider.create(properties, context); + return provider.create(properties, withConnectorConfig(context, provider)); } } LOG.debug("No ConnectorProvider supports catalogType='{}' (standaloneOnly={}). Registered: {}", @@ -312,6 +356,27 @@ private Connector createConnector(String catalogType, Map proper return null; } + /** + * Attaches the chosen provider's plugin conf to the context handed to {@code create}. + * + *

    It has to happen here rather than in the context itself: fe-core builds a + * {@link ConnectorContext} for a catalog before it knows which plugin will claim the type, so the + * conf can only be layered on once the provider is picked. Keeping it out of + * {@code DefaultConnectorContext} is also what keeps the engine's context free of any connector's + * key names. + * + *

    A sibling connector ({@code ConnectorContext.createSiblingConnector}) comes back through this + * same method, so it is handed its own plugin's conf rather than inheriting the gateway's. + * That is the intent: the conf belongs to the plugin, not to the catalog. + * + *

    An empty conf is not wrapped. The interface default already answers an empty map, and one fewer + * decorator is one fewer place a future {@link ConnectorContext} method can be lost in. + */ + private ConnectorContext withConnectorConfig(ConnectorContext base, ConnectorProvider provider) { + Map conf = connectorConfigs.get(provider); + return conf == null || conf.isEmpty() ? base : new ConnectorConfigContext(base, conf); + } + /** * Finds the provider that would back a catalog of this type, without creating a connector. For engine * decisions that must be answered for a catalog that may not be initialized yet — asking the connector diff --git a/fe/fe-core/src/main/java/org/apache/doris/connector/DefaultConnectorContext.java b/fe/fe-core/src/main/java/org/apache/doris/connector/DefaultConnectorContext.java index 4f3adf1ef6f53b..6439784c1ecaab 100644 --- a/fe/fe-core/src/main/java/org/apache/doris/connector/DefaultConnectorContext.java +++ b/fe/fe-core/src/main/java/org/apache/doris/connector/DefaultConnectorContext.java @@ -572,6 +572,17 @@ private static String withTrailingSlash(String uri) { return uri.endsWith("/") ? uri : uri + "/"; } + /** + * The fe.conf values forwarded to every connector. + * + *

    Do not add to this. A key here is an engine change per connector setting, and every one + * of them ties a connector's key name into fe-core. A connector that needs a deployment-level + * setting declares it in its own {@code .conf} instead, which the engine reads generically — + * see {@code ConnectorContext.getConnectorConfig()} and {@code ConnectorConf.get}. What is left + * below is either shared by several connectors (the jdbc/hive metastore keys, kept as the fallback + * for deployments that have not moved to the per-plugin files) or not a connector setting at all + * ({@code doris_home}, {@code doris_version}). + */ private static Map buildEnvironment() { Map env = new HashMap<>(); String dorisHome = EnvUtils.getDorisHome(); @@ -581,7 +592,6 @@ private static Map buildEnvironment() { env.put("jdbc_drivers_dir", Config.jdbc_drivers_dir); env.put("force_sqlserver_jdbc_encrypt_false", String.valueOf(Config.force_sqlserver_jdbc_encrypt_false)); - env.put("jdbc_driver_secure_path", Config.jdbc_driver_secure_path); // HMS metastore client socket-timeout default (C4): the metastore-spi cannot read FE Config // (no fe-common dependency), so the FE-configured value is threaded through the environment and // applied by HmsMetaStoreProperties.toHiveConfOverrides when the user has not overridden it. diff --git a/fe/fe-core/src/test/java/org/apache/doris/connector/ConfProbeSink.java b/fe/fe-core/src/test/java/org/apache/doris/connector/ConfProbeSink.java new file mode 100644 index 00000000000000..37fb70e410ac65 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/connector/ConfProbeSink.java @@ -0,0 +1,71 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector; + +import org.apache.doris.connector.api.Connector; +import org.apache.doris.connector.api.ConnectorMetadata; +import org.apache.doris.connector.api.ConnectorSession; + +import java.util.HashMap; +import java.util.Map; + +/** + * Where the temporary probe plugins of {@link ConnectorPluginConfTest} hand back what the engine gave them. + * + *

    It sits in {@code org.apache.doris.connector} deliberately. That prefix is parent-first for the + * CONNECTOR family, so the plugin's child-first classloader delegates this class to the FE's own loader and + * both sides share one Class — and therefore one static map. A probe provider cannot report through its own + * types: those really are child-loaded (which is the point — the loader reads a plugin's declared API + * version from the jar that defines its factory class), so anything it returns is uncastable on this side. + * + *

    The {@link Connector} it hands back is likewise defined here rather than in the plugin jar, so the jar + * needs to carry nothing but the provider itself. + */ +public final class ConfProbeSink { + + private static final Map> SEEN = new HashMap<>(); + + private ConfProbeSink() { + } + + /** Records the connector config a provider was handed, and gives it a connector to return. */ + public static Connector record(String type, Map connectorConfig) { + SEEN.put(type, new HashMap<>(connectorConfig)); + return new ProbeConnector(); + } + + /** What the provider of {@code type} was handed, or null if it was never asked to create anything. */ + public static Map seen(String type) { + return SEEN.get(type); + } + + public static void reset() { + SEEN.clear(); + } + + private static final class ProbeConnector implements Connector { + @Override + public ConnectorMetadata getMetadata(ConnectorSession session) { + return null; + } + + @Override + public void close() { + } + } +} diff --git a/fe/fe-core/src/test/java/org/apache/doris/connector/ConnectorPluginConfTest.java b/fe/fe-core/src/test/java/org/apache/doris/connector/ConnectorPluginConfTest.java new file mode 100644 index 00000000000000..e1de13a64a9a7a --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/connector/ConnectorPluginConfTest.java @@ -0,0 +1,253 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connector; + +import org.apache.doris.connector.api.Connector; +import org.apache.doris.connector.api.ConnectorMetadata; +import org.apache.doris.connector.api.ConnectorSession; +import org.apache.doris.connector.spi.ConnectorContext; +import org.apache.doris.connector.spi.ConnectorProvider; +import org.apache.doris.connectorconf.testplugins.ConfProbeConnectorProviderA; +import org.apache.doris.connectorconf.testplugins.ConfProbeConnectorProviderB; +import org.apache.doris.extension.loader.ApiVersionGate; +import org.apache.doris.extension.loader.PluginRegistry; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; + +import java.io.IOException; +import java.io.InputStream; +import java.nio.charset.StandardCharsets; +import java.nio.file.Files; +import java.nio.file.Path; +import java.util.Collections; +import java.util.Map; +import java.util.jar.Attributes; +import java.util.jar.JarEntry; +import java.util.jar.JarOutputStream; +import java.util.jar.Manifest; + +/** + * A connector plugin's own {@code .conf} really reaches the connector, on real plugin directories. + * + *

    Everything here goes through {@code loadPlugins} + {@code createConnector} rather than the + * {@code registerDiscovered} seam the rest of {@link ConnectorPluginManagerTest} uses, because the two + * things worth proving — that the file is found next to the jars, and that each provider gets its own + * file — only exist on that path. + */ +public class ConnectorPluginConfTest { + + @TempDir + Path tempDir; + + private ConnectorPluginManager manager; + + @BeforeEach + @AfterEach + void reset() { + // loadPlugins writes inventory rows into a process-wide singleton; leaving them behind would make + // information_schema.extensions assertions in other tests depend on execution order. + PluginRegistry.getInstance().clearForTest(); + ConfProbeSink.reset(); + manager = new ConnectorPluginManager(); + } + + @Test + public void connectorReceivesTheConfShippedNextToItsJars() throws IOException { + Path root = pluginRoot(); + deployPlugin(root, ConfProbeConnectorProviderA.class, ConfProbeConnectorProviderA.TYPE, + "drivers_dir=/opt/drivers\ntimeout=30\n"); + + manager.loadPlugins(Collections.singletonList(root)); + manager.createConnector(ConfProbeConnectorProviderA.TYPE, Collections.emptyMap(), context()); + + Map seen = ConfProbeSink.seen(ConfProbeConnectorProviderA.TYPE); + Assertions.assertNotNull(seen, "the probe provider was never asked to create a connector"); + Assertions.assertEquals("/opt/drivers", seen.get("drivers_dir")); + Assertions.assertEquals("30", seen.get("timeout")); + } + + @Test + public void confIsNamedAfterTheProviderNotAfterTheDirectory() throws IOException { + // The plugin directory name is the deployer's choice; the conf file name is the plugin's own + // identity. hive/hms.conf ships exactly this way, so a file named after the directory must NOT be + // picked up -- otherwise renaming a plugin directory would silently change which file is read. + Path root = pluginRoot(); + Path dir = deployPlugin(root, ConfProbeConnectorProviderA.class, + ConfProbeConnectorProviderA.TYPE, null); + Files.write(dir.resolve("some_plugin_dir.conf"), + "drivers_dir=/wrong\n".getBytes(StandardCharsets.UTF_8)); + + manager.loadPlugins(Collections.singletonList(root)); + manager.createConnector(ConfProbeConnectorProviderA.TYPE, Collections.emptyMap(), context()); + + Assertions.assertEquals(Collections.emptyMap(), + ConfProbeSink.seen(ConfProbeConnectorProviderA.TYPE), + "only .conf may be read"); + } + + @Test + public void missingConfLeavesTheProviderRegisteredWithAnEmptyMap() throws IOException { + // Shipping no conf at all is the normal case for a connector whose settings have defaults or a + // fe.conf fallback. It must cost the plugin nothing. + Path root = pluginRoot(); + deployPlugin(root, ConfProbeConnectorProviderA.class, ConfProbeConnectorProviderA.TYPE, null); + + manager.loadPlugins(Collections.singletonList(root)); + + Assertions.assertTrue(manager.getRegisteredTypes().contains(ConfProbeConnectorProviderA.TYPE), + manager.getRegisteredTypes().toString()); + Assertions.assertNotNull( + manager.createConnector(ConfProbeConnectorProviderA.TYPE, Collections.emptyMap(), context())); + Assertions.assertEquals(Collections.emptyMap(), + ConfProbeSink.seen(ConfProbeConnectorProviderA.TYPE)); + } + + @Test + public void unreadableConfLeavesTheProviderRegisteredWithAnEmptyMap() throws IOException { + // A broken file must not make the catalog type disappear: CREATE CATALOG would then answer "no + // provider supports type", which points nowhere near the real cause. The connector proceeds + // without the file and falls back to fe.conf. + Path root = pluginRoot(); + deployPlugin(root, ConfProbeConnectorProviderA.class, ConfProbeConnectorProviderA.TYPE, + "k=\\uZZZZ\n"); + + manager.loadPlugins(Collections.singletonList(root)); + + Assertions.assertTrue(manager.getRegisteredTypes().contains(ConfProbeConnectorProviderA.TYPE), + "a bad conf file must not cost the deployment its catalog type"); + manager.createConnector(ConfProbeConnectorProviderA.TYPE, Collections.emptyMap(), context()); + Assertions.assertEquals(Collections.emptyMap(), + ConfProbeSink.seen(ConfProbeConnectorProviderA.TYPE)); + } + + @Test + public void onePluginsConfNeverReachesAnother() throws IOException { + // The map is keyed by provider instance, and this is the assertion that keeps it that way. It is + // also what makes a sibling connector correct: createSiblingConnector comes back through + // createConnector, so a gateway's sibling is handed its OWN plugin's conf, not the gateway's. + Path root = pluginRoot(); + deployPlugin(root, ConfProbeConnectorProviderA.class, ConfProbeConnectorProviderA.TYPE, + "shared_key=from_a\nonly_in_a=yes\n"); + deployPlugin(root, ConfProbeConnectorProviderB.class, ConfProbeConnectorProviderB.TYPE, + "shared_key=from_b\n"); + + manager.loadPlugins(Collections.singletonList(root)); + manager.createConnector(ConfProbeConnectorProviderA.TYPE, Collections.emptyMap(), context()); + manager.createConnector(ConfProbeConnectorProviderB.TYPE, Collections.emptyMap(), context()); + + Map seenA = ConfProbeSink.seen(ConfProbeConnectorProviderA.TYPE); + Map seenB = ConfProbeSink.seen(ConfProbeConnectorProviderB.TYPE); + Assertions.assertEquals("from_a", seenA.get("shared_key")); + Assertions.assertEquals("from_b", seenB.get("shared_key")); + Assertions.assertNull(seenB.get("only_in_a"), "B must not see a key only A's conf declares"); + } + + @Test + public void providerWithNoPluginDirectoryGetsAnEmptyMap() { + // Classpath built-ins and providers a test registers directly have no plugin directory, so there + // is no file to read. They must fall through to the interface default rather than to a null map. + manager.registerProvider(new ConnectorProvider() { + @Override + public String getType() { + return "no_plugin_dir"; + } + + @Override + public Connector create(Map properties, ConnectorContext context) { + Assertions.assertEquals(Collections.emptyMap(), context.getConnectorConfig()); + return new Connector() { + @Override + public ConnectorMetadata getMetadata(ConnectorSession session) { + return null; + } + + @Override + public void close() { + } + }; + } + }); + + Assertions.assertNotNull( + manager.createConnector("no_plugin_dir", Collections.emptyMap(), context())); + } + + private Path pluginRoot() throws IOException { + Path root = tempDir.resolve("connector-plugins"); + Files.createDirectories(root); + return root; + } + + private static ConnectorContext context() { + return new ConnectorContext() { + @Override + public String getCatalogName() { + return "test_catalog"; + } + + @Override + public long getCatalogId() { + return 1L; + } + }; + } + + /** + * Lays out one plugin the way the assembly and build.sh really do: {@code /

    /.jar} with + * the provider's class bytes, its ServiceLoader registration and the served API version in the + * MANIFEST — plus, when {@code confContent} is given, {@code .conf} beside the jar. + * + *

    The directory is deliberately NOT named after the provider, so that nothing here can pass by + * accidentally reading a file named after the directory. + */ + private Path deployPlugin(Path root, Class providerClass, String providerName, String confContent) + throws IOException { + Path dir = root.resolve("some_plugin_dir_" + providerName); + Files.createDirectories(dir); + Path jarPath = dir.resolve("plugin.jar"); + + Manifest manifest = new Manifest(); + manifest.getMainAttributes().put(Attributes.Name.MANIFEST_VERSION, "1.0"); + manifest.getMainAttributes().putValue("Doris-Connector-Plugin-Api-Version", + ApiVersionGate.forFamily("connector", ConnectorProvider.class).getExpectedVersion()); + String classEntry = providerClass.getName().replace('.', '/') + ".class"; + try (JarOutputStream jar = new JarOutputStream(Files.newOutputStream(jarPath), manifest)) { + jar.putNextEntry(new JarEntry(classEntry)); + try (InputStream classBytes = providerClass.getClassLoader().getResourceAsStream(classEntry)) { + Assertions.assertNotNull(classBytes, "class bytes not found: " + classEntry); + byte[] buffer = new byte[8192]; + int read; + while ((read = classBytes.read(buffer)) != -1) { + jar.write(buffer, 0, read); + } + } + jar.closeEntry(); + jar.putNextEntry(new JarEntry("META-INF/services/" + ConnectorProvider.class.getName())); + jar.write((providerClass.getName() + "\n").getBytes(StandardCharsets.UTF_8)); + jar.closeEntry(); + } + if (confContent != null) { + Files.write(dir.resolve(providerName + ".conf"), confContent.getBytes(StandardCharsets.UTF_8)); + } + return dir; + } +} diff --git a/fe/fe-core/src/test/java/org/apache/doris/connectorconf/testplugins/ConfProbeConnectorProviderA.java b/fe/fe-core/src/test/java/org/apache/doris/connectorconf/testplugins/ConfProbeConnectorProviderA.java new file mode 100644 index 00000000000000..4e36a6adf7c9ce --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/connectorconf/testplugins/ConfProbeConnectorProviderA.java @@ -0,0 +1,52 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connectorconf.testplugins; + +import org.apache.doris.connector.ConfProbeSink; +import org.apache.doris.connector.api.Connector; +import org.apache.doris.connector.spi.ConnectorContext; +import org.apache.doris.connector.spi.ConnectorProvider; + +import java.util.Map; + +/** + * A connector provider whose class bytes are copied into a temporary plugin jar, so a test can load it the + * way FE loads a shipped connector and see what {@code getConnectorConfig()} delivered. + * + *

    It lives outside {@code org.apache.doris.connector.} on purpose: that prefix is parent-first, and the + * loader reads a plugin's declared API version from the jar that defines its factory class — a + * parent-first provider would always look undeclared and be refused. It reports through + * {@link ConfProbeSink}, which is parent-first and therefore shared with the test. + * + *

    {@link ConfProbeConnectorProviderB} is its twin, deployed as a second plugin, so that a test can prove + * one plugin's conf never reaches the other. + */ +public class ConfProbeConnectorProviderA implements ConnectorProvider { + + public static final String TYPE = "conf_probe_a"; + + @Override + public String getType() { + return TYPE; + } + + @Override + public Connector create(Map properties, ConnectorContext context) { + return ConfProbeSink.record(TYPE, context.getConnectorConfig()); + } +} diff --git a/fe/fe-core/src/test/java/org/apache/doris/connectorconf/testplugins/ConfProbeConnectorProviderB.java b/fe/fe-core/src/test/java/org/apache/doris/connectorconf/testplugins/ConfProbeConnectorProviderB.java new file mode 100644 index 00000000000000..c77d6b9a265496 --- /dev/null +++ b/fe/fe-core/src/test/java/org/apache/doris/connectorconf/testplugins/ConfProbeConnectorProviderB.java @@ -0,0 +1,41 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +package org.apache.doris.connectorconf.testplugins; + +import org.apache.doris.connector.ConfProbeSink; +import org.apache.doris.connector.api.Connector; +import org.apache.doris.connector.spi.ConnectorContext; +import org.apache.doris.connector.spi.ConnectorProvider; + +import java.util.Map; + +/** The second probe plugin; see {@link ConfProbeConnectorProviderA}. */ +public class ConfProbeConnectorProviderB implements ConnectorProvider { + + public static final String TYPE = "conf_probe_b"; + + @Override + public String getType() { + return TYPE; + } + + @Override + public Connector create(Map properties, ConnectorContext context) { + return ConfProbeSink.record(TYPE, context.getConnectorConfig()); + } +} diff --git a/fe/fe-filesystem/fe-filesystem-obs/src/main/java/org/apache/doris/filesystem/obs/ObsFileSystemProperties.java b/fe/fe-filesystem/fe-filesystem-obs/src/main/java/org/apache/doris/filesystem/obs/ObsFileSystemProperties.java index 60ec22fb8a3abe..f76c0ce239629e 100644 --- a/fe/fe-filesystem/fe-filesystem-obs/src/main/java/org/apache/doris/filesystem/obs/ObsFileSystemProperties.java +++ b/fe/fe-filesystem/fe-filesystem-obs/src/main/java/org/apache/doris/filesystem/obs/ObsFileSystemProperties.java @@ -417,12 +417,20 @@ private static void putIfNotBlank(Map map, String key, String va } private static boolean isClassAvailable(String className) { - try { - Class.forName(className, false, ObsFileSystemProperties.class.getClassLoader()); - return true; - } catch (ClassNotFoundException e) { - return false; - } + // Read as a resource rather than Class.forName: the question is whether the OBS connector + // ships in this plugin, and loading the class answers a strictly harder one. OBSFileSystem + // comes from hadoop-huaweicloud, which declares its hadoop-common parent provided, so + // hadoop-common is deliberately absent from this plugin's runtime closure and from lib/. + // Class.forName has to link the missing superclass org.apache.hadoop.fs.FileSystem and + // throws NoClassDefFoundError -- a LinkageError, not a ClassNotFoundException, so the + // former catch did not hold it and it aborted this class's static initializer instead. + // Widening the catch would only trade the crash for a lie: the probe would report OBS + // absent and silently downgrade fs.obs.impl to S3AFileSystem as soon as the host stops + // supplying hadoop-common, even though the connector is right there in lib/ and the + // consumers that actually instantiate it (fe-connector-paimon, be-java-extensions/ + // hadoop-deps) carry their own hadoop. Resolving the class file uses the same classloader + // and the same delegation without linking anything. + return ObsFileSystemProperties.class.getResource("/" + className.replace('.', '/') + ".class") != null; } @Override diff --git a/fe/fe-filesystem/fe-filesystem-obs/src/test/java/org/apache/doris/filesystem/obs/ObsFileSystemPropertiesTest.java b/fe/fe-filesystem/fe-filesystem-obs/src/test/java/org/apache/doris/filesystem/obs/ObsFileSystemPropertiesTest.java index 2aa9b7048b096c..a9f288e5f6ba41 100644 --- a/fe/fe-filesystem/fe-filesystem-obs/src/test/java/org/apache/doris/filesystem/obs/ObsFileSystemPropertiesTest.java +++ b/fe/fe-filesystem/fe-filesystem-obs/src/test/java/org/apache/doris/filesystem/obs/ObsFileSystemPropertiesTest.java @@ -129,6 +129,33 @@ void toMaps_emitObsTuningDefaultsWhenNotConfigured() { Assertions.assertEquals("10000", hadoopKv.get("fs.s3a.connection.timeout")); } + @Test + void hadoopMap_selectsObsFileSystemWithoutLinkingIt() { + // Premise this test rests on, and the exact shape of a deployment where fe-core no longer + // supplies hadoop: hadoop-huaweicloud puts org.apache.hadoop.fs.obs.OBSFileSystem on this + // module's classpath, while hadoop-common -- which owns its superclass + // org.apache.hadoop.fs.FileSystem -- is deliberately not part of this plugin. Loading the + // class therefore fails with NoClassDefFoundError, a LinkageError and NOT a + // ClassNotFoundException. Asserted so that adding hadoop-common later fails here loudly + // rather than quietly turning the rest of this test into a tautology. + Assertions.assertThrows(NoClassDefFoundError.class, + () -> Class.forName("org.apache.hadoop.fs.obs.OBSFileSystem", false, + ObsFileSystemProperties.class.getClassLoader())); + + // The probe asks whether the OBS connector ships in this plugin, not whether this JVM can + // link it, so an unlinkable-but-present OBSFileSystem must still select the native impl -- + // the consumers that instantiate it carry their own hadoop. Probing with Class.forName got + // both halves wrong here: it threw, and catching the LinkageError would have downgraded + // fs.obs.impl to S3AFileSystem while the connector sat in lib/. + ObsFileSystemProperties properties = ObsFileSystemProperties.of(Map.of( + "obs.endpoint", "https://obs.cn-north-4.myhuaweicloud.com")); + + Map hadoopKv = properties.toHadoopConfigurationMap(); + Assertions.assertEquals("org.apache.hadoop.fs.obs.OBSFileSystem", hadoopKv.get("fs.obs.impl")); + Assertions.assertEquals("org.apache.hadoop.fs.obs.OBS", + hadoopKv.get("fs.AbstractFileSystem.obs.impl")); + } + @Test void bind_rejectsPartialStaticCredentialsLikeFeCore() { IllegalArgumentException exception = Assertions.assertThrows(IllegalArgumentException.class, diff --git a/regression-test/suites/external_table_p0/hive/ddl/test_hive_ddl.groovy b/regression-test/suites/external_table_p0/hive/ddl/test_hive_ddl.groovy index b654819826ed27..75f92730664205 100644 --- a/regression-test/suites/external_table_p0/hive/ddl/test_hive_ddl.groovy +++ b/regression-test/suites/external_table_p0/hive/ddl/test_hive_ddl.groovy @@ -462,7 +462,7 @@ suite("test_hive_ddl", "p0,external") { 'replication_num' = '1' ); """ - exception "Create hive bucket table need set enable_create_hive_bucket_table to true" + exception "Create hive bucket table need set 'enable_create_bucket_table' in hms.conf (or enable_create_hive_bucket_table in fe.conf) to true" } sql """ SWITCH internal """ diff --git a/run-fe-ut.sh b/run-fe-ut.sh index 27adbc745e52f6..a941a4a7651a70 100755 --- a/run-fe-ut.sh +++ b/run-fe-ut.sh @@ -36,6 +36,49 @@ is_valid_extra_module_feature() { [[ "${feature}" =~ ^[A-Za-z][A-Za-z0-9_-]*$ ]] } +# The CI job parses FE UT results with the report pattern fe/*/target/surefire-reports/*.xml. That +# single wildcard only reaches the modules sitting directly under fe/; every module nested one level +# deeper -- fe-filesystem/*, fe-connector/*, fe-authentication/* -- is invisible to it. Combined +# with the -Dmaven.test.failure.ignore=true that the coverage run needs in order to finish every +# module and still emit a jacoco report, a failing nested module leaves maven at exit 0, the reactor +# printing SUCCESS for it, and the job green with "failed: 0". +# +# So gate here on the reports the CI parser cannot see. The ones it can see are deliberately left to +# it: it owns those results, and its per-test mutes have to keep working. +fail_on_unparsed_test_failures() { + local report module header failures errors + local -a broken=() + + while IFS= read -r report; do + # The module path relative to fe/, e.g. "fe-core" or "fe-filesystem/fe-filesystem-obs". + # No slash in it means the module sits directly under fe/, which is exactly what the CI + # pattern's single wildcard reaches -- leave those to the CI parser. Deliberately not + # written as a [[ ]] glob against the pattern itself: there, * also matches /, so + # fe/*/target/... would swallow the nested modules this function exists to catch. + module="${report#"${DORIS_HOME}"/fe/}" + module="${module%%/target/*}" + [[ "${module}" != */* ]] && continue + + # The totals live on the root element. -m1 so that a stack trace quoted inside + # some later can never be mistaken for it. + header="$(grep -m1 -o ']*>' "${report}")" || continue + failures="$(sed -n 's/.*failures="\([0-9]*\)".*/\1/p' <<<"${header}")" + errors="$(sed -n 's/.*errors="\([0-9]*\)".*/\1/p' <<<"${header}")" + + if [[ "${failures:-0}" -gt 0 || "${errors:-0}" -gt 0 ]]; then + broken+=("${report#"${DORIS_HOME}/"} -- failures=${failures:-0} errors=${errors:-0}") + fi + done < <(find "${DORIS_HOME}/fe" -type f -path '*/target/surefire-reports/*.xml') + + if [[ "${#broken[@]}" -ne 0 ]]; then + echo "" + echo "FE UT failed in ${#broken[@]} test class(es) whose module the CI report pattern does not reach:" + printf ' %s\n' "${broken[@]}" + echo "" + return 1 + fi +} + parse_extra_fe_modules() { local spec_value="$1" local entry feature module_path existing @@ -202,4 +245,9 @@ else "${MVN_CMD}" test -pl "${MVN_MODULES}" -am -Dcheckstyle.skip=true -DfailIfNoTests=false \ -Dmaven.build.cache.enabled=false fi + + # Only reachable when maven itself exited 0, which under -Dmaven.test.failure.ignore=true it + # does even with failing tests. Deliberately not run for --run: that invocation leaves every + # other module's reports from an earlier run untouched, and those are not this run's results. + fail_on_unparsed_test_failures fi