morningman commented on code in PR #66729:
URL: https://github.com/apache/doris/pull/66729#discussion_r4004152607
##########
be/src/format/transformer/vfile_format_transformer_factory.cpp:
##########
@@ -49,8 +49,23 @@ Status create_tvf_format_transformer(const TTVFTableSink&
tvf_sink, RuntimeState
if (tvf_sink.__isset.line_delimiter) {
writer_params["line_delimiter"] = tvf_sink.line_delimiter;
}
- result->reset(new VJniFormatTransformer(state, output_vexpr_ctxs,
tvf_sink.writer_class,
- std::move(writer_params)));
+ // writer_class names a plugin factory, not a Java class. A class name
stopped being able
+ // to identify a writer when plugins were isolated: a concrete writer
lives in its own
+ // plugin's classloader, which BE cannot search by name, so what is
addressable is the
+ // plugin directory under plugins/jni and the factory inside it.
+ const std::string& writer = tvf_sink.writer_class;
+ const size_t sep = writer.find(':');
+ if (sep == std::string::npos || sep == 0 || sep + 1 == writer.size()) {
Review Comment:
The JNI writer path (`writer_type = JNI`, selected by the `writer_class`
property) is an experimental feature: it was added on master by #60756 and has
not shipped in any release - no 4.x branch carries it. There is no
rolling-upgrade compatibility window to preserve for it, so the
`<plugin>:<factory>` spelling is the only one going forward and the legacy
class-name spelling is rejected rather than mapped.
##########
fe/fe-connector/fe-connector-spi/src/main/java/org/apache/doris/connector/spi/ConnectorMetadata.java:
##########
@@ -98,6 +98,46 @@ default Optional<ConnectorMvccPartitionView>
getMvccPartitionView(
return Optional.empty();
}
+ /**
+ * Whether {@link #listPartitions} reads the pin carried by a
snapshot-applied handle, so that a
+ * {@code FOR TIME/VERSION AS OF} query can be told which partitions
existed AT that pin.
+ *
+ * <p>Point-in-time time travel otherwise pins with EMPTY partition maps,
because a partition set
+ * listed at LATEST is the wrong universe for a past snapshot in both
directions: it hides a
+ * partition that has since been dropped (pruning it away loses rows) and
invents ones created
+ * after the pin. Empty is the safe answer — the generic scan node reads
it as scan-all and lets
+ * the connector's own predicate pushdown do the pruning — but it costs
the query its partition
+ * pruning and makes EXPLAIN report {@code partition=0/0} for a scan that
reads everything.</p>
+ *
+ * <p>A connector that answers true promises that {@code listPartitions}
on the handle returned by
+ * {@link #applySnapshot} enumerates exactly the partitions with data at
that pin. The generic
+ * model then pins the real partition set and both pruning and {@code
partition=N/M} become
+ * truthful. The default is false: a connector whose listing is
snapshot-blind keeps the empty pin,
+ * which is correct, just coarse.</p>
+ *
+ * <p><b>A SECOND PROMISE COMES WITH IT, and it is easy to miss:</b> the
AT-SNAPSHOT schema this
+ * connector returns from {@link #getTableSchema(ConnectorSession,
ConnectorTableHandle,
+ * ConnectorMvccSnapshot)} must declare, through {@code
ConnectorTableSchema.PARTITION_COLUMNS_KEY},
+ * partition columns that MATCH the values that pinned listing produces -
same count, in the same
+ * order, each parseable into the column's type. That schema is what types
the pinned partition
+ * items, because it is the schema this snapshot publishes; the latest
schema may partition the
+ * table differently.</p>
+ *
+ * <p>Breaking that promise degrades quietly rather than failing: each
mismatched partition is
+ * skipped inside a per-partition catch, the pinned item map ends up
shorter than the listed name
+ * set, and the table is reported UNPARTITIONED. Rows are still correct -
pruning and
+ * {@code partition=N/M} are what is lost. Two signals in the log: one
WARN per skipped
+ * partition, and - when EVERY partition was skipped, which is what a
schema that never matched
+ * produces as opposed to iceberg spec evolution - one aggregate WARN
naming both counts and the
+ * partition columns, from {@code PluginDrivenMvccExternalTable}. Note
+ * that a connector whose at-snapshot schema resolution can degrade to an
empty column list on
+ * error - hudi's {@code getSchemaFromMetaClient} swallows a failed
metadata read into one -
+ * produces exactly this shape from a transient fault.</p>
+ */
+ default boolean listsPartitionsAtSnapshot(ConnectorSession session,
ConnectorTableHandle handle) {
Review Comment:
The connector SPI itself is unreleased: fe-connector exists on master only
and no 4.x branch carries it, so there are no shipped connector plugins for a
major bump to protect. We are not bumping the version for this addition;
`listsPartitionsAtSnapshot` is recorded in the `connector-metadata-methods.txt`
baseline and in the version test's note for the current major.
##########
fe/be-java-extensions/jni-bootstrap/src/main/java/org/apache/doris/jni/bootstrap/PluginRuntime.java:
##########
@@ -0,0 +1,657 @@
+// 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.jni.bootstrap;
+
+import org.apache.doris.jni.spi.DorisPlugin;
+import org.apache.doris.jni.spi.JniScannerFactory;
+import org.apache.doris.jni.spi.JniWriterFactory;
+import org.apache.doris.jni.spi.SpiVersion;
+import org.apache.doris.jni.spi.ThreadContextClassLoader;
+import org.apache.doris.jni.spi.UdfExecutorFactory;
+import org.apache.doris.jni.spi.utils.JniUtil;
+
+import java.io.IOException;
+import java.net.URL;
+import java.net.URLClassLoader;
+import java.nio.file.DirectoryStream;
+import java.nio.file.Files;
+import java.nio.file.Path;
+import java.util.ArrayList;
+import java.util.Comparator;
+import java.util.Iterator;
+import java.util.LinkedHashMap;
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.ServiceLoader;
+import java.util.TreeMap;
+import java.util.concurrent.ConcurrentHashMap;
+import java.util.logging.Level;
+import java.util.logging.Logger;
+
+/**
+ * Loads plugins from a directory and hands BE the objects it asks for.
+ *
+ * <p>Loading is lazy and happens at most once per plugin: the first request
for a plugin builds its
+ * classloader, runs {@link ServiceLoader} and indexes the factories, and
every later request reads
+ * the cached {@link PluginHandle} - including a cached failure. Nothing is
eagerly resolved beyond
+ * the plugin object and its factories, so a class missing from a plugin's
jars is reported when
+ * something touches it, with the name of the class, instead of taking the
process down at startup.
+ *
+ * <p>All plugin code runs with the plugin's classloader installed as the
thread context
+ * classloader. BE's threads have no meaningful one, and ServiceLoader and
most plugin libraries
+ * consult it.
+ */
+final class PluginRuntime {
+
+ private static final Logger LOG =
Logger.getLogger(PluginRuntime.class.getName());
+
+ private final Path pluginDir;
+ private final ClassLoader spiClassLoader;
+ private final ClassLoader hadoopConfResources;
+ private final Path fsDir;
+ private final ConcurrentHashMap<String, PluginHandle> plugins = new
ConcurrentHashMap<>();
+ private final ConcurrentHashMap<String, Object> loadLocks = new
ConcurrentHashMap<>();
+
+ PluginRuntime(Path pluginDir, ClassLoader spiClassLoader) {
+ this(pluginDir, spiClassLoader, null, null);
+ }
+
+ PluginRuntime(Path pluginDir, ClassLoader spiClassLoader, Path
hadoopConfDir) {
+ this(pluginDir, spiClassLoader, hadoopConfDir, null);
+ }
+
+ /**
+ * @param hadoopConfDir directory whose files every plugin can read as
classpath resources, so
+ * that a hadoop {@code Configuration} built inside a
plugin finds
+ * {@code core-site.xml} and friends. Null when there
is none.
+ * @param fsDir directory of third-party hadoop {@code FileSystem}
jars every plugin
+ * may need; see {@link #sharedFilesystemJars()}.
Null when there is none.
+ */
+ PluginRuntime(Path pluginDir, ClassLoader spiClassLoader, Path
hadoopConfDir, Path fsDir) {
+ this.pluginDir = Objects.requireNonNull(pluginDir, "pluginDir");
+ this.spiClassLoader = Objects.requireNonNull(spiClassLoader,
"spiClassLoader");
+ this.hadoopConfResources = hadoopConfLoader(hadoopConfDir);
+ this.fsDir = fsDir;
+ }
+
+ /**
+ * Jars appended to EVERY plugin's classpath, after that plugin's own.
+ *
+ * <p>What lives here: third-party hadoop {@code FileSystem}
implementations that no plugin
+ * declares as a dependency because none of them is written against it -
JindoFS serves
+ * {@code oss://} and {@code oss-hdfs://}, JuiceFS serves {@code jfs://},
and hadoop reaches
+ * both by class name out of a {@code Configuration}. Both are opt-in
build flags
+ * ({@code DISABLE_BUILD_JINDOFS=OFF}, {@code DISABLE_BUILD_JUICEFS=OFF}),
so on a default
+ * build this directory is absent and this method returns nothing.
+ *
+ * <p>Why shared rather than bundled per plugin. Before the plugins were
isolated these jars
+ * sat on the system classpath, which every scanner could reach, so any
table format could read
+ * a table on any of those filesystems. A plugin classloader cannot reach
that classpath by
+ * design, and the alternative - copying the jars into each plugin
directory - does not scale:
+ * the JuiceFS Hadoop SDK is a 180 MB fat jar that carries jersey,
checkerframework and
+ * javax.ws.rs, which collide with about 1500 classes already in a
lake-format plugin. One
+ * directory read by every plugin costs one copy on disk and produces no
collisions to
+ * adjudicate.
+ *
+ * <p>APPENDED, never prepended: these fat jars carry stray copies of
third-party classes,
+ * hadoop's included, and a plugin's own hadoop must win. Counted on the
jars this build
+ * packages: jindo-sdk carries 19 hadoop classes (11 in {@code
org.apache.hadoop.fs}, 5 in
+ * {@code fs.impl}, 3 in {@code util}) and juicefs-hadoop carries 4, all in
+ * {@code org.apache.hadoop.security}. That is the same rule {@code
bin/start_be.sh} applies
+ * when it puts them after {@code lib/hadoop_hdfs} on the system classpath
for libhdfs.
+ *
+ * <p>ISOLATION IS PRESERVED: each plugin loads its own copy of these
classes in its own
Review Comment:
The premise does not hold for the jindo-core that Doris packages.
`com.aliyun.jindodata.jnative.NativeCodeLoader.loadLibrary(File,
NativeLibContext)` in jindo-core 6.10.4 (the version in `thirdparty/vars.sh`)
catches the `UnsatisfiedLinkError` whose message contains `already loaded in
another classloader`, copies the extracted library to a `<name>-<UUID>.so` next
to it, `System.load`s that copy for the asking classloader, and deletes the
file afterwards - it is written for exactly the several-classloaders-per-JVM
case. I verified this by disassembling the jar with `javap` (the older 6.3.4
jar under `docker/thirdparties` has the same handling). A second plugin, or a
plugin next to libhdfs on the system classpath, therefore gets a binding of its
own instead of a failure.
The CAVEAT comments in `PluginRuntime.java` and `build.sh` that stated the
single-owner limitation were wrong; they are corrected to describe this
behaviour.
--
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]
