morningman commented on code in PR #66729: URL: https://github.com/apache/doris/pull/66729#discussion_r4005218664
########## fe/be-java-extensions/jni-bootstrap/src/main/java/org/apache/doris/jni/bootstrap/DorisPluginClassLoader.java: ########## @@ -0,0 +1,212 @@ +// 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 java.io.IOException; +import java.net.URL; +import java.net.URLClassLoader; +import java.util.ArrayList; +import java.util.Collections; +import java.util.Enumeration; +import java.util.List; +import java.util.Objects; + +/** + * Classloader of one plugin. It splits the class space into three parts, and which part a name + * falls into is decided by the name alone: + * + * <ol> + * <li><b>JDK</b> - the parent is the <em>platform</em> classloader, not the system one. This is + * the load-bearing choice: BE's system classpath (conf, the SPI jars, and the hadoop drop + * that C++ libhdfs needs) is not reachable from a plugin at all.</li> + * <li><b>SPI</b> - names under {@code org.apache.doris.jni.spi.} are resolved only through BE's + * own classloader, so BE and the plugin hold the same class identity for the types they + * exchange.</li> + * <li><b>Plugin</b> - everything else comes from the plugin's own directory.</li> + * </ol> + * + * <p>Classes are split exactly that way. <em>Resources</em> have one addition: the hadoop conf drop + * point is searched after the plugin's own jars, because (1) also puts every {@code .xml} on BE's + * classpath out of reach and a hadoop {@code Configuration} inside a plugin has to find its site + * files somewhere. See the constructor. + * + * <p>The point of (1) is that isolation is structural rather than a matter of search order. A + * child-first loader that falls back to its parent still lets a class the plugin failed to package + * resolve against whatever BE happens to ship, which is how two versions of hadoop end up mixed + * inside one plugin with nothing reporting it. Here the same mistake is a + * {@link ClassNotFoundException} naming the class, and the build-time jdeps gate catches it before + * that. + * + * <p>Modelled on Trino's {@code io.trino.server.PluginClassLoader}, including the two diagnostics + * below, which exist because "SPI class not found" has exactly two causes and they need different + * fixes. + */ +public class DorisPluginClassLoader extends URLClassLoader { + + /** Prefix delegated to BE's classloader. Also the reason SPI classes live in that package. */ + static final String SPI_PACKAGE = "org.apache.doris.jni.spi."; + + /** Resource form of {@link #SPI_PACKAGE}. */ + static final String SPI_RESOURCE_PREFIX = "org/apache/doris/jni/spi/"; + + static { + registerAsParallelCapable(); + } + + private final String pluginName; + private final ClassLoader spiClassLoader; + private final ClassLoader hadoopConfResources; + + public DorisPluginClassLoader(String pluginName, List<URL> urls, ClassLoader spiClassLoader) { + this(pluginName, urls, spiClassLoader, null); + } + + /** + * @param pluginName the plugin's directory name, used in diagnostics + * @param urls the jars in that directory + * @param spiClassLoader BE's own classloader, the only place SPI classes may come from + * @param hadoopConfResources loader over the hadoop conf drop point, consulted for resources + * only, or null. Because (1) above cuts a plugin off from BE's + * classpath, it also cuts it off from every {@code .xml} on it - + * and a hadoop {@code Configuration} built inside a plugin looks + * {@code core-site.xml} up as a resource through this loader. So + * resources, and only resources, have one more place to come from. + */ + public DorisPluginClassLoader(String pluginName, List<URL> urls, ClassLoader spiClassLoader, + ClassLoader hadoopConfResources) { + // Plugins must not see the system (application) classloader. + super(urls.toArray(new URL[0]), getPlatformClassLoader()); Review Comment: Fixed in 2dc1fc2b4ee. The loader's parent stays the platform loader; the patched class travels with the plugin instead. paimon-scanner, iceberg-metadata-scanner and hadoop-hudi-scanner now declare `hadoop-deps` (every transitive excluded, so only the patched `FileSystem` joins the closure), and the jar's new `Doris-Shadows-Classes` manifest entry makes `PluginRuntime` search it before every other jar in the plugin directory - by name, `hadoop-common-*.jar` would win. `check_plugin_layout.py` holds the attribute to its word: a shadowing jar must carry exactly the classes it names, and only those may duplicate a sibling's. Verified three ways: `PluginRuntimeTest` on synthetic jars (a shadowing jar that sorts last still wins; jars without the attribute keep name order), each plugin's `keysTheFilesystemCacheByTheFingerprintFeSends` (two `doris.fs.cache.key.file` values key two `file:///` entries - the vanilla class fails it), and loading the deployed paimon/iceberg/hudi directories through `PluginRuntime`, where `FileSystem` now resolves from `hadoop-deps-*.jar` in all three. Hudi's per-configuration UGI stays, but as the handle to close with (see `HadoopHudiJniScanner.java:87`), not as the credential separation. ########## fe/fe-connector/fe-connector-hudi/src/main/java/org/apache/doris/connector/hudi/HudiScanPlanProvider.java: ########## @@ -993,6 +1099,7 @@ private Configuration buildHadoopConf() { conf.set(key, entry.getValue()); } } + HudiConnector.enableFileSystemCache(conf); Review Comment: Fixed in 2dc1fc2b4ee. `HudiConnector.getScanPlanProvider()` hands the provider the connector's execute-wrapper (unwrapped for `RuntimeException`, so `planScan`'s own failures reach the engine as they did before), and the provider runs the whole of `planScan` and the metaClient of `getScanNodeProperties` inside it, so their filesystems are cached under `fileSystemScope()` - or the Kerberos authenticator's UGI - and `close()` releases them with the rest. `HudiConnectorFileSystemScopeTest.theScanPlanProviderPlansUnderTheConnectorsScope` pins that the executor the connector hands over runs under its scope and that `close()` brings the hold back to zero; `HudiScanPlanProviderScopeTest` plans a real empty table through a `file://` filesystem that records the UGI it was opened under and asserts it is the executor's for both entry points (and that force_jni opens nothing). ########## fe/fe-connector/fe-connector-hudi/src/main/java/org/apache/doris/connector/hudi/HudiConnectorMetadata.java: ########## @@ -560,6 +560,30 @@ private ConnectorMvccSnapshot resolveIncremental(HudiTableHandle handle, Map<Str return builder.build(); } + /** + * True only where the listing really is snapshot-exact. + * + * <p>When it is, {@link #listPartitions} reads the {@code queryInstant} {@link #applySnapshot} put on the + * handle and enumerates the partitions that hold data at it, so a {@code FOR TIME/VERSION AS OF} query gets + * the partition universe of THAT snapshot rather than an empty one. See + * {@code HudiScanPlanProvider.listPartitionPathsAsOf}: it is NOT the same view call the scan makes - it + * asks {@code getLatestFileSlicesBeforeOrOn} while the scan asks its own per-table-type views - but the + * two share the file group set and the {@code <= queryInstant} cut, and every difference falls on the safe + * side, so this listing can only be a superset of the partitions the scan finds files in. Its own javadoc + * carries that argument in full. + * + * <p>The {@code use_hive_sync_partition} branch cannot promise that, which is why it answers false. + * {@link #collectPartitions} starts from what HMS holds NOW and can only remove from it, so its result is a + * SUBSET of the pin: a partition that held data at the pin but was later dropped from the table and + * unsynced from HMS is not in that subset, and pruning against it would silently drop rows a + * {@code FOR TIME AS OF} query must read. False means "this listing knows nothing about snapshots", which + * leaves the pinned partition set empty and scans everything - coarse, but never short. + */ + @Override + public boolean listsPartitionsAtSnapshot(ConnectorSession session, ConnectorTableHandle handle) { + return !useHiveSyncPartition(); Review Comment: Fixed in 2dc1fc2b4ee. A pinned read (the engine stamps `queryInstant` in `doInitialize`, before `applyFilter` runs) no longer prunes against HMS at all: `applyFilter` prunes the Hudi metadata listing, which is the universe the unpruned pinned scan walks itself (`resolvePartitions` -> `listAllPartitionPaths`, then the file-system view at the instant), so it cannot lose a partition the scan would have read - and it costs no extra listing, since `resolvePartitions` short-circuits on the pruned set. Latest reads on hive-sync tables prune HMS names as before. `HudiPartitionPruningTest` covers both shapes (HMS knows only KEEP while the metadata listing still has GONE: pinned keeps GONE, latest prunes to nothing), and `test_hudi_partition_prune` gains `FOR TIME AS OF '<insert>' WHERE part1 = 'GONE'` / `'KEEP'` plus the latest `GONE` read, under both values of `use_hive_sync_partition`. ########## fe/be-java-extensions/hadoop-hudi-scanner/src/main/java/org/apache/doris/hudi/HadoopHudiJniScanner.java: ########## @@ -65,6 +77,15 @@ public class HadoopHudiJniScanner extends JniScanner { private static final String HADOOP_CONF_PREFIX = "hadoop_conf."; + // fs.s3a.impl.disable.cache and its per-scheme siblings, as the FE emits them. + private static final Pattern FS_DISABLE_CACHE = Pattern.compile("fs\\..+\\.impl\\.disable\\.cache"); + + // One UGI per distinct filesystem configuration, which is what keys Hadoop's FileSystem cache to + // the credentials that opened it. See createFileSystemScope. Never evicted on purpose: an entry is + // one UGI, there is one per catalog storage config, and dropping one would strand the filesystems + // cached under it - a live scan may still be reading through them. + private static final ConcurrentHashMap<String, UserGroupInformation> FS_SCOPES = new ConcurrentHashMap<>(); Review Comment: Fixed in 2dc1fc2b4ee. The map is now `HudiFileSystemScopes`: each scope counts the scanners holding it (acquired in `openInternal`, released in `closeInternal` - a failed open releases too, a double close releases once), and a sweeper closes a scope's filesystems through `FileSystem.closeAllForUGI` once it has had no holder for ten minutes. The idle window is what keeps it a cache: scanners are per split, the count reaches zero between every two queries, and closing on zero would rebuild the S3 client per query. Removal happens under the acquire lock and the close outside it, so a scan either holds the entry or gets a fresh UGI - `UserGroupInformation` equality is Subject identity, so the entries being closed can never be handed to it. `HudiFileSystemScopesTest` drives the clock and the sweep by hand: held scopes are never swept, idle ones only after the TTL, a hold taken during the window cancels the eviction, and 50 rounds of a scan racing the sweep end with the scan holding a live entry either way. `HadoopHudiPluginTest` pins the hold-while-open window on a real table. With the patched `FileSystem` now inside the plugin the UGI carries no credential separation any more, only the handle to close with; Kerberos keeps the authenticator's UGI and no scope, as before. -- 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]
