github-actions[bot] commented on code in PR #65644:
URL: https://github.com/apache/doris/pull/65644#discussion_r3600047872
##########
fe/fe-core/src/main/java/org/apache/doris/fs/FileSystemPluginManager.java:
##########
@@ -80,9 +84,18 @@ public class FileSystemPluginManager {
public void loadBuiltins() {
ServiceLoader.load(FileSystemProvider.class)
.forEach(p -> {
- providers.add(p);
-
DatasourcePrintableMap.registerSensitiveKeys(p.sensitivePropertyKeys());
- LOG.info("Registered built-in filesystem provider: {}",
p.name());
+ try {
+ // Snapshot self-reported metadata before publishing
the provider
+ // so one throwing implementation is rejected cleanly
instead of
+ // aborting startup or being active without an
inventory row.
+
PluginRegistry.getInstance().registerBuiltin(PLUGIN_FAMILY, p);
Review Comment:
[P2] Keep registry publication after sensitive-key discovery.
sensitivePropertyKeys() is overridable provider code and can throw; because
this row is inserted first, the catch below skips the provider but cannot
remove the record, so information_schema.plugins permanently advertises a
BUILTIN provider that the runtime manager rejected. Snapshot the sensitive keys
before mutating either store (and honor one registration decision atomically).
The external loop needs the same failure ordering/cleanup: it currently adds
the provider before this callback and before the registry row.
##########
fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/DirectoryPluginRuntimeManager.java:
##########
@@ -265,22 +288,90 @@ 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(factory.getClass(), allJars);
+
return new PluginHandle<>(
pluginName.trim(),
normalizedDir,
allJars,
classLoader,
factory,
- Instant.now());
+ Instant.now(),
+ description,
+ implementationVersion);
+ }
+
+ /**
+ * Reads Implementation-Version from the MANIFEST of the jar that defined
the
+ * factory class: the class's code source when available (covers layouts
where
+ * the service descriptor sits in a root jar but the implementation lives
in
+ * lib/), otherwise the first candidate jar containing the class entry.
+ * Version is display-only metadata: failures degrade to null instead of
+ * failing the load.
+ */
+ private String readImplementationVersion(Class<?> factoryClass, List<Path>
candidateJars) {
+ Path definingJar = jarOf(factoryClass);
+ if (definingJar != null) {
+ try (JarFile jarFile = new JarFile(definingJar.toFile())) {
+ return manifestImplementationVersion(jarFile);
+ } catch (IOException ignored) {
+ // Fall through to scanning the candidate jars.
+ }
+ }
+ String classEntry = factoryClass.getName().replace('.', '/') +
".class";
+ for (Path jar : candidateJars) {
+ try (JarFile jarFile = new JarFile(jar.toFile())) {
+ if (jarFile.getEntry(classEntry) == null) {
+ continue;
+ }
+ return manifestImplementationVersion(jarFile);
+ } catch (IOException ignored) {
+ // Display-only metadata; fall through to the next candidate
jar.
+ }
+ }
+ return null;
+ }
+
+ private static String manifestImplementationVersion(JarFile jarFile)
throws IOException {
+ Manifest manifest = jarFile.getManifest();
+ return manifest == null ? null
Review Comment:
[P2] Honor package-specific Implementation-Version attributes. Java
manifests allow package versioning in a named section such as Name:
com/acme/plugin/, and that value overrides the main section for classes in that
package. This helper reads only main attributes, so a factory with a valid
package-section version is reported with the wrong main value or NULL. Read the
factory package section first with main attributes as fallback, and cover that
manifest layout.
##########
fe/fe-extension-spi/src/main/java/org/apache/doris/extension/spi/PluginFactory.java:
##########
@@ -31,6 +31,17 @@ public interface PluginFactory {
*/
String name();
+ /**
+ * Returns a one-line human-readable description of the plugin.
+ *
+ * <p>Snapshotted once at load time by the loader; never invoked at query
time.</p>
+ *
+ * @return plugin description, empty by default
+ */
+ default String description() {
Review Comment:
[P1] Preserve compatibility for existing factory jars. A factory compiled
against the old SPI can legally inherit description() from an unrelated
interface; adding this default creates two maximally-specific implementations
and can make the new call throw IncompatibleClassChangeError. The loader
catches only RuntimeException, so this LinkageError escapes per-plugin
isolation without closing the classloader and can abort the family load or FE
startup. Put optional metadata on a new subinterface with an empty fallback for
old factories, and add an old-SPI binary compatibility fixture.
--
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]
