github-actions[bot] commented on code in PR #66729: URL: https://github.com/apache/doris/pull/66729#discussion_r4004691607
########## 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: [P1] Preserve the Doris filesystem cache key in isolated plugins This platform-only parent keeps plugins from seeing `hadoop-deps`, while the Paimon and Iceberg closures package ordinary `hadoop-common`. FE now relies on `doris.fs.cache.key.<scheme>` instead of disabling Hadoop's cache, but vanilla `FileSystem.Cache.Key` ignores that property. Their simple-auth scanners share a UGI by `hadoop.username`; for example, two Paimon catalogs using `hdfs://ns` with different `dfs.namenode.rpc-address.ns.*` maps make the second scan reuse the first catalog's `DistributedFileSystem` and route to the wrong cluster. Hudi adds a per-configuration UGI to avoid this collision; Paimon and Iceberg do not. Put the patched `FileSystem` behavior in each affected plugin closure (without splitting Hadoop type identity), or add equivalent per-configuration scopes, and test through the deployed plugin loaders. ########## 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: [P1] Release scan-planning filesystems with the connector This enables Hadoop caching for `planScan` and `getScanNodeProperties`, but those calls run under the FE login UGI (the engine only pins the plugin classloader). `HudiConnector.close()` closes filesystems under its separate metadata scope, so every altered/rotated catalog fingerprint leaves this planning entry, its SDK client, and executor threads resident until FE restart. Repeated ALTER or CREATE/query/DROP cycles can therefore recreate the native-thread exhaustion this change is meant to stop. Run these provider I/O paths under the connector's ref-counted scope, or otherwise evict their exact cache entries on final close, and cover planning followed by ALTER/DROP. ########## 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: [P1] Bound the BE-side Hudi filesystem scopes This process-static map is deliberately never evicted. Each distinct `hadoop_conf.*` map gets a new UGI, and this scanner turns Hadoop caching back on under that UGI; `closeInternal()` only closes the record reader, so the cached filesystem, SDK client, and executor threads for every old credential/configuration remain until BE restart. Repeated ALTER/credential rotation or CREATE/query/DROP cycles therefore turn the prior per-query leak into an unbounded per-ever-used-configuration leak. Reference-count active scanners and close/remove the scope after its final owner (or use a bounded idle cache), with a concurrent lifecycle test so eviction cannot strand a live scan. ########## 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: [P1] Do not preserve latest-HMS pruning on a pinned hive-sync read Returning false here only empties the generic pinned partition universe; it does not disable Hudi's connector-handle pruning. `applyFilter` receives a handle that already carries `queryInstant`, but its hive-sync branch still prunes partition predicates against current HMS and stores `prunedPartitionPaths`; later pinning preserves that list, while `resolvePartitions` treats a non-null empty list as zero scan paths. If `GONE` existed at the requested instant but was later dropped and unsynced, `FOR TIME AS OF '<old>' WHERE part1 = 'GONE'` therefore returns zero rows. Skip current-HMS pruning for pinned hive-sync reads (or recompute it from a snapshot-aware source), and add this predicate to the dropped-partition regression. -- 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]
