github-actions[bot] commented on code in PR #67770:
URL: https://github.com/apache/doris/pull/67770#discussion_r3978332587
##########
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"
+ mkdir -p "${SHARED_LIB_DIR}"
+
HADOOP_RUNTIME_ZIP="${DORIS_HOME}/fe/fe-hadoop-runtime/target/doris-fe-hadoop-runtime.zip"
+ if [[ -f "${HADOOP_RUNTIME_ZIP}" ]]; then
+ # Same rule as the plugin directories: unzip -o overwrites but never
removes, so a version
+ # bump would leave both copies of every versioned jar here and the
layer would bind whichever
+ # the sorted URL order reached first. Clear what the zip owns and
unpack fresh.
+ rm -rf "${SHARED_LIB_DIR}/hadoop/lib"
+ rm -f "${SHARED_LIB_DIR}/hadoop"/*.jar
+ mkdir -p "${SHARED_LIB_DIR}/hadoop"
+ unzip -q -o "${HADOOP_RUNTIME_ZIP}" -d "${SHARED_LIB_DIR}/hadoop/"
Review Comment:
[P2] Avoid shipping the shared bundle before it has a consumer
On this head, `SharedLibraryLayer.resolve()` has no production call site.
`ConnectorPluginManager` and `FileSystemPluginManager` still pass their
application classloader directly to `DirectoryPluginRuntimeManager`, and `Env`
scans only `plugins/connector` and `plugins/filesystem`. This line therefore
adds a 29-jar bundle to every FE output alongside the unchanged kernel/plugin
copies, but no runtime path can use it. Please either wire the shared parent in
this change, with a production-topology test, or defer building and unpacking
the bundle until the consumer lands.
##########
fe/fe-core/pom.xml:
##########
@@ -743,6 +770,22 @@ under the License.
<artifactId>hadoop-aws</artifactId>
</dependency>
+ <!-- Compiled against directly, but reaching fe-core only as a hadoop
transitive:
+ metrics-core (com.codahale.metrics) via hadoop-auth, carrying the
whole metric layer -
+ MetricRepo, the MetricVisitors, HistogramMetric, CloudMetrics,
SqlBlockRule - and
+ bcprov-jdk18on (org.bouncycastle.util) via hadoop-common, used by
TableScanParams.
+ Declared here so that neither is a passenger of a dependency that
says nothing about
+ them, and so that dropping hadoop from the kernel does not
silently take them along.
+ Both are already on this classpath at these versions; no jar is
added to fe/lib. -->
+ <dependency>
+ <groupId>io.dropwizard.metrics</groupId>
+ <artifactId>metrics-core</artifactId>
+ </dependency>
+ <dependency>
+ <groupId>org.bouncycastle</groupId>
+ <artifactId>bcprov-jdk18on</artifactId>
Review Comment:
[P1] Account for this dependency in the required license check
The exact-head `dependency-review` job fails on this newly direct
`org.bouncycastle:bcprov-jdk18on` dependency as `LicenseRef-bad-non-standard`.
Doris already records Bouncy Castle under MIT in
`dist/licenses/LICENSE.bouncycastle.txt` and `dist/NOTICE-dist.txt`, but
`.github/workflows/third_party_review.yml` has no package-specific exception
for this coordinate. Please add the narrow exception, or otherwise make the
approved license metadata visible, so the required gate can pass.
##########
fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/SharedLibraryLayer.java:
##########
@@ -0,0 +1,157 @@
+// 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 java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+/**
+ * A library layer shared by every plugin classloader of every family.
+ *
+ * <p>Some libraries cannot be bundled per plugin. A library whose classes
inherit from one another
+ * across jars has to be loaded once or the JVM refuses the link; one that
holds process-wide static
+ * state (a client cache, a login context, a first-caller-wins registry) has
to be loaded once or two
+ * plugins silently stop sharing it; one with a JNI native image can only be
bound to a single
+ * classloader per process. Hadoop is all three at once. Giving each plugin
its own copy is therefore
+ * not "the same thing, duplicated" - it changes behavior.
+ *
+ * <p>So such libraries are installed once, under a shared root, and this
class turns that root into
+ * a single classloader that is then the PARENT of every plugin classloader:
+ *
+ * <pre>
+ * app classloader (fe/lib)
+ * └── shared library layer <- plugins/shared/<bundle>/*.jar
+ lib/*.jar
+ * ├── filesystem plugin (child-first)
+ * ├── connector plugin (child-first)
+ * └── ...
+ * </pre>
+ *
+ * <p>The layer is itself child-first with only the mandatory parent-first
prefixes, so a bundle uses
+ * its own dependencies where it has them and falls back to fe/lib otherwise;
logging stays the
+ * kernel's, which keeps the bundle's output in fe.log. Plugins reach the
bundle by the ordinary
+ * child-first fallback: a plugin that carries its own copy of the library
keeps using it and is
+ * simply not sharing, which is the pre-existing behavior for a third-party
plugin.
+ *
+ * <h2>Layout</h2>
+ *
+ * <p>{@code <root>/<bundle>/*.jar} then {@code <root>/<bundle>/lib/*.jar},
bundles in name order -
+ * the same convention {@link DirectoryPluginRuntimeManager} uses for a plugin
directory. Root jars
+ * before {@code lib/} is what lets a bundle ship a patched copy of a class
that also exists in one
+ * of its dependency jars: put the patch in the bundle root and the stock jar
under {@code lib/}.
+ *
+ * <h2>Absence is normal</h2>
+ *
+ * <p>A root that does not exist, or holds no jar, yields {@code parent}
unchanged, so a deployment
+ * that installs no bundle keeps exactly the classloader graph it had before
this class existed. An
+ * unreadable root is not that case and is raised, because degrading it to
"nothing installed" turns
+ * a permissions mistake into a missing-class failure much later and somewhere
else.
+ *
+ * <h2>One instance per root</h2>
+ *
+ * <p>Memoized on the resolved root, for the whole process. This is the point
of the class rather
+ * than an optimization: two callers that got two layers over the same jars
would get two copies of
+ * every class in them, which is the situation the layer exists to prevent.
The first caller's parent
+ * is the one that ends up in the graph - there is one FE app classloader, so
every caller passes the
+ * same one. The layer is never closed; it lives as long as the process, like
the app classloader.
+ */
+public final class SharedLibraryLayer {
+
+ private static final ConcurrentMap<Path, ClassLoader> LAYERS = new
ConcurrentHashMap<>();
+
+ private SharedLibraryLayer() {
+ }
+
+ /**
+ * Returns the classloader for the shared bundles under {@code root}, or
{@code parent} itself
+ * when there is nothing to install there.
+ *
+ * @param root the shared library root; null is treated as "not configured"
+ * @param parent the classloader the layer delegates to, normally the FE
app classloader
+ * @throws UncheckedIOException if the root exists but cannot be read
+ */
+ public static ClassLoader resolve(Path root, ClassLoader parent) {
+ Objects.requireNonNull(parent, "parent");
+ if (root == null) {
+ return parent;
+ }
+ Path resolved = root.toAbsolutePath().normalize();
Review Comment:
[P2] Canonicalize the physical root before memoizing
`toAbsolutePath().normalize()` removes lexical `..` components but does not
resolve symlinks. If one physical bundle root is reached through both a release
symlink and its real path, the unequal keys each create a child-first loader
over the same jars, producing duplicate Hadoop class identity and static state
-- the exact condition this layer is meant to prevent. Canonicalize an existing
root with `toRealPath()` before `computeIfAbsent`, preserving the documented
I/O failure behavior, and add a symlink-alias identity test.
##########
fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/DirectoryPluginRuntimeManager.java:
##########
@@ -523,6 +534,38 @@ private String discoverSingleFactoryClassName(Class<F>
factoryType, ClassLoader
return classNames.get(0);
}
+ /**
+ * Turns a load failure caused by an absent class into an actionable
sentence, or "" for any other
+ * failure. A plugin misses a class either because it does not bundle it
or because it expected to
+ * inherit it from the layer its classloader delegates to, and the message
must not leave the reader
+ * guessing which - a shared library bundle that was never installed looks
exactly like a broken
+ * plugin jar otherwise.
+ */
+ private static String missingClassAdvice(Throwable failure) {
+ String missing = missingClassName(failure);
+ if (missing == null) {
+ return "";
+ }
+ return ". The class " + missing + " is in neither this plugin's own
jars nor its parent"
+ + " classloader; if it is meant to come from a shared library
bundle, check that the"
+ + " bundle is installed under the FE shared library root";
+ }
+
+ /** The absent class named by a NoClassDefFoundError /
ClassNotFoundException anywhere in the chain. */
+ private static String missingClassName(Throwable failure) {
+ for (Throwable t = failure; t != null; t = t.getCause() == t ? null :
t.getCause()) {
+ if (t instanceof NoClassDefFoundError || t instanceof
ClassNotFoundException) {
Review Comment:
[P2] Do not report initializer failures as a missing class named `Could`
After a class initializer has failed, a later JVM linkage attempt reports
`NoClassDefFoundError: Could not initialize class X`. This branch accepts that
error and line 562 returns its first whitespace token, so the new advice says
that a class named `Could` is absent and recommends checking the shared bundle
even though the class was found. Recognize the initialization-failure form
before extracting a binary name, inspect the cause chain only for a genuine
missing class, and add a two-attempt initializer test.
##########
fe/fe-extension-loader/src/main/java/org/apache/doris/extension/loader/SharedLibraryLayer.java:
##########
@@ -0,0 +1,157 @@
+// 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 java.io.IOException;
+import java.io.UncheckedIOException;
+import java.net.MalformedURLException;
+import java.net.URL;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.List;
+import java.util.Objects;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.concurrent.ConcurrentMap;
+import java.util.function.Predicate;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+/**
+ * A library layer shared by every plugin classloader of every family.
+ *
+ * <p>Some libraries cannot be bundled per plugin. A library whose classes
inherit from one another
+ * across jars has to be loaded once or the JVM refuses the link; one that
holds process-wide static
+ * state (a client cache, a login context, a first-caller-wins registry) has
to be loaded once or two
+ * plugins silently stop sharing it; one with a JNI native image can only be
bound to a single
+ * classloader per process. Hadoop is all three at once. Giving each plugin
its own copy is therefore
+ * not "the same thing, duplicated" - it changes behavior.
+ *
+ * <p>So such libraries are installed once, under a shared root, and this
class turns that root into
+ * a single classloader that is then the PARENT of every plugin classloader:
+ *
+ * <pre>
+ * app classloader (fe/lib)
+ * └── shared library layer <- plugins/shared/<bundle>/*.jar
+ lib/*.jar
+ * ├── filesystem plugin (child-first)
+ * ├── connector plugin (child-first)
+ * └── ...
+ * </pre>
+ *
+ * <p>The layer is itself child-first with only the mandatory parent-first
prefixes, so a bundle uses
+ * its own dependencies where it has them and falls back to fe/lib otherwise;
logging stays the
+ * kernel's, which keeps the bundle's output in fe.log. Plugins reach the
bundle by the ordinary
+ * child-first fallback: a plugin that carries its own copy of the library
keeps using it and is
+ * simply not sharing, which is the pre-existing behavior for a third-party
plugin.
+ *
+ * <h2>Layout</h2>
+ *
+ * <p>{@code <root>/<bundle>/*.jar} then {@code <root>/<bundle>/lib/*.jar},
bundles in name order -
+ * the same convention {@link DirectoryPluginRuntimeManager} uses for a plugin
directory. Root jars
+ * before {@code lib/} is what lets a bundle ship a patched copy of a class
that also exists in one
+ * of its dependency jars: put the patch in the bundle root and the stock jar
under {@code lib/}.
+ *
+ * <h2>Absence is normal</h2>
+ *
+ * <p>A root that does not exist, or holds no jar, yields {@code parent}
unchanged, so a deployment
+ * that installs no bundle keeps exactly the classloader graph it had before
this class existed. An
+ * unreadable root is not that case and is raised, because degrading it to
"nothing installed" turns
+ * a permissions mistake into a missing-class failure much later and somewhere
else.
+ *
+ * <h2>One instance per root</h2>
+ *
+ * <p>Memoized on the resolved root, for the whole process. This is the point
of the class rather
+ * than an optimization: two callers that got two layers over the same jars
would get two copies of
+ * every class in them, which is the situation the layer exists to prevent.
The first caller's parent
+ * is the one that ends up in the graph - there is one FE app classloader, so
every caller passes the
+ * same one. The layer is never closed; it lives as long as the process, like
the app classloader.
+ */
+public final class SharedLibraryLayer {
+
+ private static final ConcurrentMap<Path, ClassLoader> LAYERS = new
ConcurrentHashMap<>();
+
+ private SharedLibraryLayer() {
+ }
+
+ /**
+ * Returns the classloader for the shared bundles under {@code root}, or
{@code parent} itself
+ * when there is nothing to install there.
+ *
+ * @param root the shared library root; null is treated as "not configured"
+ * @param parent the classloader the layer delegates to, normally the FE
app classloader
+ * @throws UncheckedIOException if the root exists but cannot be read
+ */
+ public static ClassLoader resolve(Path root, ClassLoader parent) {
+ Objects.requireNonNull(parent, "parent");
+ if (root == null) {
+ return parent;
+ }
+ Path resolved = root.toAbsolutePath().normalize();
+ return LAYERS.computeIfAbsent(resolved, dir -> build(dir, parent));
+ }
+
+ private static ClassLoader build(Path root, ClassLoader parent) {
+ if (!Files.isDirectory(root)) {
Review Comment:
[P2] Preserve attribute I/O failures instead of caching an empty layer
`Files.isDirectory` and `Files.isRegularFile` return `false` when attributes
cannot be read. A root that can be listed but whose children cannot be searched
can therefore have every bundle entry silently filtered out; `build()` returns
`parent`, and `computeIfAbsent` caches that false no-op for the process
lifetime even after permissions are repaired. This contradicts the documented
fail-loud behavior for an unreadable root. Use attribute reads that preserve
`IOException` for the root, bundle, `lib`, and jar probes, wrap failures as
documented, and add a permission-failure/non-memoization test.
##########
fe/fe-core/src/test/java/org/apache/doris/common/FeCoreHasNoHadoopClassesTest.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.common;
+
+import org.apache.doris.catalog.Env;
+
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+import java.io.IOException;
+import java.nio.charset.StandardCharsets;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.List;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
+
+/**
+ * The FE kernel must carry no Hadoop.
+ *
+ * <p>Hadoop belongs to the filesystem and connector plugins, which load it
through their own
+ * classloaders; fe-core itself must not reference a single {@code
org.apache.hadoop} type. Removing
+ * the jars from {@code fe/lib} is only half of that — the other half is that
no fe-core class names
+ * one, because javac needs nothing but the class on the compile classpath and
a reference that
+ * compiles today keeps compiling after the jar leaves, failing instead at
runtime with
+ * {@code NoClassDefFoundError} on whichever code path first reaches it.
+ *
+ * <p>This scans fe-core's own compiled output rather than its source: a
constant-pool entry is the
+ * form a dependency actually takes, so it catches the reference no import
reveals — a supertype
+ * inherited from another module, a type that only appears in a method
descriptor, a synthetic
+ * bridge. The needles are in JVM internal form ({@code org/apache/hadoop/}),
which is how class
+ * references and field/method descriptors are spelled; the dotted spelling is
deliberately NOT
+ * matched, because {@link org.apache.doris.connector.ConnectorPluginManager}
and
+ * {@link org.apache.doris.fs.FileSystemPluginManager} legitimately hold
{@code "org.apache.hadoop."}
+ * as a parent-first classloader-policy string.
+ *
+ * <p>{@code org/apache/doris/kerberos/} is a needle too. That module splits
into a Hadoop half
+ * (UGI logins, {@code AuthenticationConfig(Configuration)}) and a Hadoop-free
remainder, and its
+ * Hadoop half would sit in {@code fe/lib} as classes that can never link
there. fe-core takes the
+ * one interface it needs, {@code ExecutionAuthenticator}, from fe-foundation
instead, and
+ * {@code fe-core/pom.xml} no longer depends on fe-kerberos at all.
+ */
+public class FeCoreHasNoHadoopClassesTest {
+
+ /** JVM internal form: how class references and descriptors are spelled in
the constant pool. */
+ private static final List<String> FORBIDDEN = Arrays.asList(
Review Comment:
[P2] Cover reflective dotted names in the Hadoop-free guard
This raw-byte scan rejects only JVM-internal slash names. A reflective
target such as `Class.forName("org.apache.hadoop.conf.Configuration")` or
`loadClass("org.apache.doris.kerberos.AuthenticationConfig")` is stored as a
dotted `CONSTANT_String`, passes this test, and can then fail at runtime after
those kernel jars disappear. fe-core already has reflective loading paths.
Please reject the dotted prefixes too, narrowly allowlist the two legitimate
parent-first policy owners, and add a reflective negative-control 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]
