github-actions[bot] commented on code in PR #65644:
URL: https://github.com/apache/doris/pull/65644#discussion_r3592314622
##########
fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorPluginManager.java:
##########
@@ -75,6 +79,7 @@ public void loadBuiltins() {
ServiceLoader.load(ConnectorProvider.class)
.forEach(p -> {
providers.add(p);
+
PluginRegistry.getInstance().registerBuiltin(PLUGIN_FAMILY, p);
Review Comment:
[P2] Isolate the new metadata callback before publishing the provider.
registerBuiltin() invokes the overridable description() here without a
per-provider catch, after providers.add(p). One throwing implementation can
therefore abort FE startup/discovery or leave an active provider with no
inventory row; the external loader already treats this callback as a per-plugin
failure and cleans up. Please snapshot metadata under the same failure boundary
before mutating runtime state, then either reject that provider cleanly or
deliberately fall back to empty metadata.
##########
fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/DirectoryPluginRuntimeManager.java:
##########
@@ -265,22 +270,62 @@ private PluginHandle<F> loadFromPluginDir(Path pluginDir,
ClassLoader parent, Cl
"Failed to get plugin name from discovered factory in " +
normalizedDir,
e);
}
- if (pluginName == null || pluginName.trim().isEmpty()) {
+ String nameValidationError = PluginNames.validate(pluginName);
Review Comment:
[P1] Preserve the existing external-plugin name contract. Before this change
the loader accepted every nonblank name and stored its trimmed value, while the
public SPI/guide only require a stable nonempty identity. This validator now
rejects deployed names such as acme/auth:v2 (and all non-ASCII names or names
over 64 chars) during FE startup, even though this PR is adding inventory
metadata. Please keep the existing acceptance contract, or introduce this
restriction with an explicit compatibility/migration plan.
##########
fe/fe-authentication/fe-authentication-handler/src/main/java/org/apache/doris/authentication/handler/AuthenticationPluginManager.java:
##########
@@ -88,7 +92,10 @@ public
AuthenticationPluginManager(DirectoryPluginRuntimeManager<AuthenticationP
: ClassLoadingPolicy.defaultPolicy();
ServiceLoader.load(AuthenticationPluginFactory.class)
- .forEach(factory -> factories.put(factory.name(), factory));
+ .forEach(factory -> {
+ factories.put(factory.name(), factory);
Review Comment:
[P1] Make runtime selection and inventory publication use one canonical
decision. This map uses the raw name and last-wins put, while PluginRegistry
trims/validates the name and keeps the first value; its boolean result is
ignored. With two ServiceLoader factories named ldap, getFactory() executes the
second but information_schema.plugins describes the first. A name rejected by
the registry can likewise remain executable yet absent from the table. Please
normalize/validate once and apply the same precedence atomically to both stores
(and the parallel family managers).
##########
fe/fe-extension-loader/src/test/java/org/apache/doris/extension/loader/PluginRegistryTest.java:
##########
@@ -0,0 +1,145 @@
+// 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.extension.loader;
+
+import org.apache.doris.extension.loader.PluginRegistry.PluginRecord;
+import org.apache.doris.extension.loader.PluginRegistry.PluginSource;
+import org.apache.doris.extension.spi.Plugin;
+import org.apache.doris.extension.spi.PluginFactory;
+
+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 java.util.List;
+
+class PluginRegistryTest {
+
+ @BeforeEach
+ void setUp() {
+ PluginRegistry.getInstance().clearForTest();
Review Comment:
[P2] Anchoring here because the production registry has no textual diff:
keep PluginRegistry.java textual. Its key separator is a literal NUL byte, so
Git classifies the entire new 197-line source as binary, normal text diffs hide
it, and ordinary text search cannot inspect it. The current compile checks
pass, but the repository/tooling regression remains. Please express the runtime
separator with a Java escape (or use a structured key) so the corrected file
appears as a normal text diff.
##########
regression-test/suites/query_p0/schema_table/test_plugins_schema.groovy:
##########
@@ -0,0 +1,63 @@
+// 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.
+
+import org.junit.Assert
+
+suite("test_plugins_schema", "p0") {
+ // Schema check: fixed five columns.
+ def schema = sql "DESC information_schema.plugins"
+ def columnNames = schema.collect { it[0] }
+ Assert.assertEquals(
+ ["PLUGIN_NAME", "PLUGIN_TYPE", "PLUGIN_VERSION", "SOURCE",
"DESCRIPTION"], columnNames)
+
+ // As an admin user: built-in plugins (e.g. filesystem providers) must be
listed.
+ def rows = sql """
+ SELECT PLUGIN_NAME, PLUGIN_TYPE, SOURCE
+ FROM information_schema.plugins
+ ORDER BY PLUGIN_TYPE, PLUGIN_NAME
+ """
+ Assert.assertTrue("expect at least one built-in plugin row", rows.size() >
0)
+ rows.each { row ->
+ Assert.assertTrue(row[2] == "BUILTIN" || row[2] == "EXTERNAL")
+ }
+ def builtinRows = rows.findAll { it[2] == "BUILTIN" }
+ Assert.assertTrue("expect built-in plugins registered", builtinRows.size()
> 0)
Review Comment:
[P1] Do not require a BUILTIN row in the standard packaged cluster. FE core
removes filesystem implementations from its runtime classpath (and
authentication's password provider is test-scope); build.sh deploys
filesystem/connector implementations under output/fe/plugins, and these
managers register those rows as EXTERNAL. A correctly populated default
inventory can therefore have zero BUILTIN rows and fail here. Please assert the
packaged external inventory (or inject a controlled built-in in a unit test).
Also convert deterministic results to qt_/order_qt_, generate the .out, and
include PLUGIN_VERSION so the advertised NULL is covered.
##########
fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/DirectoryPluginRuntimeManager.java:
##########
@@ -265,22 +270,62 @@ private PluginHandle<F> loadFromPluginDir(Path pluginDir,
ClassLoader parent, Cl
"Failed to get plugin name from discovered factory in " +
normalizedDir,
e);
}
- if (pluginName == null || pluginName.trim().isEmpty()) {
+ String nameValidationError = PluginNames.validate(pluginName);
+ if (nameValidationError != null) {
closeClassLoader(classLoader);
throw new PluginLoadException(
normalizedDir,
LoadFailure.STAGE_INSTANTIATE,
- "Plugin name is empty for directory: " + normalizedDir,
+ "Invalid plugin name for directory " + normalizedDir + ":
" + nameValidationError,
null);
}
+ String description;
+ try {
+ description = factory.description();
+ } catch (RuntimeException e) {
+ closeClassLoader(classLoader);
+ throw new PluginLoadException(
+ normalizedDir,
+ LoadFailure.STAGE_INSTANTIATE,
+ "Failed to get plugin description from discovered factory
in " + normalizedDir,
+ e);
+ }
+
+ String implementationVersion = readImplementationVersion(
Review Comment:
[P2] Look up the version in the jar that actually defined the factory.
Discovery only parses the class name from root-jar service descriptors, then
the runtime classloader can load that class from root plus lib jars. With a
root descriptor jar and the implementation in lib/plugin-impl.jar, loading
succeeds but this root-only search returns NULL. Please use the loaded class's
code source or scan all runtime jars in classloader order, and cover the split
layout.
##########
fe/fe-core/src/main/java/org/apache/doris/tablefunction/MetadataGenerator.java:
##########
@@ -795,6 +809,38 @@ private static TFetchSchemaTableDataResult
authenticationIntegrationsMetadataRes
return result;
}
+ private static TFetchSchemaTableDataResult
pluginsMetadataResult(TSchemaTableRequestParams params) {
+ if (!params.isSetCurrentUserIdent()) {
+ return errorResult("current user ident is not set.");
+ }
+ UserIdentity currentUserIdentity =
UserIdentity.fromThrift(params.getCurrentUserIdent());
+ TFetchSchemaTableDataResult result = new TFetchSchemaTableDataResult();
+ List<TRow> dataBatch = Lists.newArrayList();
+ result.setDataBatch(dataBatch);
+ result.setStatus(new TStatus(TStatusCode.OK));
+ // The plugin inventory reveals which security/storage components this
cluster
+ // runs; restrict it to ADMIN like SHOW PLUGINS.
+ if
(!Env.getCurrentEnv().getAccessManager().checkGlobalPriv(currentUserIdentity,
PrivPredicate.ADMIN)) {
+ return result;
+ }
+
+ // Registry rows are load-time snapshots of the current FE; no plugin
code runs here.
+ for (PluginRegistry.PluginRecord record :
PluginRegistry.getInstance().list()) {
+ TRow row = new TRow();
+ row.addToColumnValue(new TCell().setStringVal(record.getName()));
+ row.addToColumnValue(new TCell().setStringVal(record.getType()));
+ if (record.getVersion() == null) {
+ row.addToColumnValue(new TCell());
Review Comment:
[P1] Preserve SQL NULL end to end for unavailable versions. An unset TCell
reaches SchemaScanner::insert_block_column(), whose VARCHAR branch inserts the
default empty string and always appends false to the null map, so
PLUGIN_VERSION IS NULL is false and the advertised NULL becomes ''. Please send
the explicit null marker and make this scanner/helper insert a nullable default
when it is set, with a result-level test for a versionless built-in.
##########
fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorPluginManager.java:
##########
@@ -104,13 +109,30 @@ public void loadPlugins(List<Path> pluginRoots) {
}
for (PluginHandle<ConnectorProvider> handle : report.getSuccesses()) {
+ // Built-ins (and earlier-loaded plugins) must never be displaced
by a
+ // same-name directory jar.
+ if (hasProviderNamed(handle.getPluginName())) {
Review Comment:
[P2] Dispose the handle rejected by this family-level conflict. loadAll()
has already inserted this success into runtimeManager.handlesByName, so
continue leaves the factory and its URLClassLoader strongly retained and the
jar resources open for the FE lifetime; FileSystemPluginManager has the same
path. Please reject before publication or add an atomic remove-and-close
operation. Closing alone is insufficient while the handle remains in the
runtime map.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
