morningman commented on code in PR #67770:
URL: https://github.com/apache/doris/pull/67770#discussion_r3980477271
##########
build.sh:
##########
@@ -1181,6 +1185,29 @@ if [[ "${BUILD_FE}" -eq 1 ]]; then
mkdir -p "${DORIS_OUTPUT}/fe/plugins/hadoop_conf/"
mkdir -p "${DORIS_OUTPUT}/fe/plugins/java_extensions/"
+ # Shared library bundles: libraries that every plugin needing them must
load from ONE place,
+ # because their classes inherit across jars and their static state
(FileSystem.CACHE, the UGI
+ # login, the FileSystem service registry) is what makes two plugins agree
about one cluster.
+ # SharedLibraryLayer turns this directory into the parent classloader of
every filesystem and
+ # connector plugin. Deliberately at plugins/ level and not under
plugins/filesystem or
+ # plugins/connector: DirectoryPluginRuntimeManager treats every direct
subdirectory of those as
+ # a plugin, and would report this one as a plugin that failed to load.
+ # NOTE: plugins/shared/ is NOT added to the FE CLASSPATH - it is loaded by
FE, not by the JVM
+ # launcher. See bin/start_fe.sh.
+ SHARED_LIB_DIR="${DORIS_OUTPUT}/fe/plugins/shared"
Review Comment:
Thanks — the unconditional `mkdir` is a fair catch and is fixed. The rest
I'd like to push back on, with the evidence.
**Fixed:** the outer `mkdir -p "${SHARED_LIB_DIR}"` is gone. The `mkdir -p
"${SHARED_LIB_DIR}/hadoop"` inside the `if` already builds the whole chain, so
`plugins/shared` now exists only when there is a bundle to put in it; a build
without the zip no longer occupies the name for nothing.
**Not moving the directory out of `Config.plugin_dir`.** Two reasons.
First, the name space this is said to break is already reserved nine times
over, on master, by this same file. `build.sh` creates `jdbc_drivers`,
`adbc_drivers`, `java_udf`, `trino_plugins`, `hadoop_conf`, `java_extensions`
with an unconditional `mkdir -p`, and `filesystem`, `connector`,
`authorization` from their deploy loops — all under
`${DORIS_OUTPUT}/fe/plugins`, which is what `Config.plugin_dir` resolves to.
`PluginInfo.readFromProperties()` has never validated a name beyond non-empty,
so a legacy plugin called `connector` or `authorization` is un-installable on
master today, with the same error. This PR adds a tenth name to that set; it
does not create the collision class. This review excluded two other findings
for predating the PR, and by that standard this one is a pre-existing property
of the packaged layout.
Second, the placement is deliberate and the comment on the hunk says why:
`DirectoryPluginRuntimeManager` treats every direct subdirectory of
`plugins/filesystem` and `plugins/connector` as a plugin, so a bundle under
either would be reported as a plugin that failed to load. Moving it out of
`Config.plugin_dir` means moving it out of `plugins/` altogether, which costs
the symmetry with the other nine reserved names.
**One correction on the mechanism.** `movePlugin()` is not reached on that
path. `PluginMgr.installPlugin()` calls `getPluginInfo()` before `install()`,
so `pluginInfo` is already set when `install()` runs its first check, and
`hasInstalled()` (`DynamicPluginLoader.java:108`, `:129-138`) throws `Plugin
shared has already been installed.` before `movePlugin()` is called. Same
outcome, different line.
**Where the real fix belongs.** If the project wants this closed rather than
documented, the fix is a reserved-name check in
`PluginInfo.readFromProperties()` or `PluginMgr.installPlugin()` that rejects
all ten packaged names with a message that says what actually happened, instead
of the misleading "has already been installed". That repairs the nine cases
already on master as well as this one, and it is a change to the legacy
dynamic-plugin loader rather than to this PR's packaging — so it deserves its
own PR and its own install/replay coverage, not a hunk here.
Worth recording while it is in view: for anyone who did have a legacy plugin
named after one of these ten, `UNINSTALL PLUGIN <name>` would reach
`DynamicPluginLoader.uninstall()` (`:143-153`) with `installPath` pointing at
the packaged directory and delete it. The other two paths are safe — a failed
fresh install deletes only the `.install_*` temp directory, and
`replayLoadDynamicPlugin()` sets `ERROR` and rethrows without calling
`uninstall()`. That hazard is identical for the nine names on master and is one
more argument for fixing this in the loader.
--
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]
