github-actions[bot] commented on code in PR #67770:
URL: https://github.com/apache/doris/pull/67770#discussion_r4021331447
##########
fe/fe-core/src/main/java/org/apache/doris/connector/ConnectorPluginManager.java:
##########
@@ -220,8 +239,23 @@ void loadBuiltins(ClassLoader classLoader) {
* @return true if the provider was admitted
*/
boolean registerDiscovered(ConnectorProvider provider, boolean failFast) {
- String type = provider.getType();
- Set<String> engineNames = provider.acceptedCreateTableEngineNames();
+ String type;
+ Set<String> engineNames;
+ try {
+ type = provider.getType();
+ engineNames = provider.acceptedCreateTableEngineNames();
+ } catch (RuntimeException | LinkageError e) {
Review Comment:
[P2] Snapshot plugin-owned engine names inside the failure guard
This guard only calls `acceptedCreateTableEngineNames()`; the returned
plugin-owned `Set` is first traversed later in `createTableEngineNameProblem()`
and then traversed again after `claimedTypes` is mutated. A lazy/custom set
whose iterator links a missing optional class (or otherwise throws) therefore
escapes `registerDiscovered()`, aborts the remaining connector plugins, and
leaves this handle undiscarded; a second-pass failure also leaves the type name
claimed. Please materialize the set into a host-owned snapshot inside this same
guard, validate/publish that snapshot, and test an iterator-time failure.
##########
fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/DirectoryPluginRuntimeManager.java:
##########
@@ -293,11 +316,20 @@ private PluginHandle<F> loadFromPluginDir(Path pluginDir,
ClassLoader parent, Cl
@SuppressWarnings("unchecked")
Class<? extends F> factoryClass = (Class<? extends F>)
discoveredClass.asSubclass(factoryType);
factory = factoryClass.getDeclaredConstructor().newInstance();
- } catch (ReflectiveOperationException e) {
+ } catch (ReflectiveOperationException | RuntimeException |
LinkageError e) {
+ // newInstance() is where the factory class is first
initialized. A static initializer
Review Comment:
[P2] Contain ServiceLoader configuration failures per plugin
`ServiceConfigurationError` is an `Error`, but not a `LinkageError`, so a
factory initializer or `name()`/`description()` callback that performs a
malformed nested `ServiceLoader` lookup still escapes this new catch.
`loadAll()` catches only `PluginLoadException`, which means one bad directory
plugin aborts the rest of the family load and this runtime classloader is never
closed. The built-in connector/lineage paths already treat this standard
provider failure as rejectable. Please include `ServiceConfigurationError` in
every new external-plugin guard (generic factory/metadata plus family
admission/init/cleanup), convert it to the normal per-plugin failure/cleanup
path, and cover a malformed nested service lookup.
##########
fe/fe-core/src/main/java/org/apache/doris/analysis/StorageDesc.java:
##########
@@ -110,6 +114,17 @@ public StorageAdapter getStorageAdapter() {
@Override
public void gsonPostProcess() throws IOException {
- initStorageAdapter();
+ // Binding runs plugin code: bindPrimary probes every loaded provider
and bind() belongs to
+ // the one that claims the map. This method runs at image load and
journal replay for every
+ // persisted load and export job, so with that provider absent - a
filesystem plugin that
+ // failed to load - a throw here would take the image load down, or
kill a serving follower
+ // at the next OP_CREATE_LOAD_JOB. Leave the adapter unbound instead:
every getter re-runs
+ // initStorageAdapter() lazily, so the job binds at use and fails
there with a Status.
+ try {
+ initStorageAdapter();
+ } catch (RuntimeException | LinkageError e) {
+ LOG.warn("Storage descriptor (name={}, type={}) could not bind its
filesystem provider at"
Review Comment:
[P2] Contain service-configuration failures during replay binding
`ServiceConfigurationError` is an `Error`, not a `LinkageError`, so an
admitted filesystem provider that performs a property-dependent nested
`ServiceLoader` lookup from `supportsExplicit()`, `supportsGuess()`, or
`bind()` still escapes this new guard. Because `gsonPostProcess()` runs while
persisted load/export jobs are restored, that aborts image load or follower
journal replay instead of leaving the descriptor unbound for the documented
retry-at-use path. This is post-admission, so fixing the directory-loader
boundary does not cover it. Please catch `ServiceConfigurationError` explicitly
at these provider-backed persistence boundaries and add a `StorageDesc`
round-trip case whose provider throws it while probing or binding.
--
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]
