[improvement](fe) Stop fe-common depending on Hadoop and pin fe-core's hidden Hadoop passengers - #67770
[improvement](fe) Stop fe-common depending on Hadoop and pin fe-core's hidden Hadoop passengers#67770morningman wants to merge 19 commits into
Conversation
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
2de0320 to
5b5d21a
Compare
|
run buildall |
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Requesting changes on exact head 5b5d21a07fd030ecad1500ba346d10d496372cec. Six PR-local findings are inline. Existing review threads were empty; two real but pre-existing plugin-boundary defects were excluded after authoritative-base comparison.
Checkpoint conclusions:
- Goal and proof: the dependency/authenticator decoupling is otherwise coherent, but the new architecture guard is incomplete, the shared-layer groundwork violates two of its own contracts, and the required dependency-review check fails. The shared bundle is deliberately not wired, so its 29 jars add distribution cost without delivering the class-identity benefit yet.
- Scope and clarity: direct dependency declarations, auth-key relocation, and dead pre-auth pool removal are focused; always packaging the unused bundle is not.
- Concurrency:
ConcurrentHashMap.computeIfAbsentmakes creation atomic and no new thread/lock ordering problem was found, but lexical path identity can still create two loaders for one physical root. - Lifecycle: the shared loader's process lifetime is intentional; caching an erroneous no-op result after suppressed attribute failures makes that lifetime harmful. Catalog authenticator reset/close behavior remains consistent.
- Configuration: no user-facing configuration or dynamic-setting semantics were added. The shared directory is an implicit packaged path.
- Compatibility and rolling behavior: auth-key values and connector/filesystem SPI behavior are preserved.
ExternalCatalog's return descriptor changes interface owner, but all actual consumers are internal/recompiled and supported plugins do not link fe-core. No storage, wire, or rolling-upgrade format change was found. - Parallel paths and conditions: filesystem, connector, authorization, lineage, Kerberos/simple-auth, Maven profiles, bundle ordering, and reused-output cleanup were traced. No additional PR-local defect survived.
- Tests and results: the new tests cover eager linkage and basic loader ordering, but omit reflective dotted dependencies, physical aliases, permission failures, and retry-form initializer diagnostics. No local build/test was run because this review task forbids builds. Live CI has CheckStyle, formatter, repository license, secrets, and large-file checks passing;
dependency-reviewfails on bcprov; FE UT and compile remain pending at review time. - Observability: load summaries are adequate, but the new missing-class parser can emit a false class name and bundle recommendation.
- Persistence, data writes, and FE/BE variables: no EditLog/storage-format/data-write/transaction or FE-BE variable path changes.
- Performance: the unused 29-jar bundle is installed into every FE output while all existing copies remain.
- Security:
SECURITY.mdand the threat model were applied because authentication code changed. Admin-installed plugin code is trusted in this model, and no in-scope security vulnerability was found.
All three convergence lanes returned NO_NEW_VALUABLE_FINDINGS in the final allowed round. Review status: complete for this head, changes requested.
| 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/" |
There was a problem hiding this 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.
| </dependency> | ||
| <dependency> | ||
| <groupId>org.bouncycastle</groupId> | ||
| <artifactId>bcprov-jdk18on</artifactId> |
There was a problem hiding this 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.
| if (root == null) { | ||
| return parent; | ||
| } | ||
| Path resolved = root.toAbsolutePath().normalize(); |
There was a problem hiding this 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.
| 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( |
There was a problem hiding this 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.
| /** 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) { |
There was a problem hiding this 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.
| } | ||
|
|
||
| private static ClassLoader build(Path root, ClassLoader parent) { | ||
| if (!Files.isDirectory(root)) { |
There was a problem hiding this 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 UT Coverage ReportIncrement line coverage |
5b5d21a to
058ab76
Compare
|
/review |
There was a problem hiding this comment.
Requesting changes on exact head 058ab76a3d94d600e583adc9b1a5ba58fa393062. One new PR-local finding is inline. Existing review threads were treated as hard duplicate fences; the two additional loader gaps found during review were excluded after authoritative base/head comparison showed that they predate this PR.
Checkpoint conclusions:
- Goal and proof: the dependency/authenticator decoupling, Hadoop-free bytecode guard, shared-layer implementation, and bundle construction are internally coherent, but the new bundle location breaks the accepted legacy dynamic-plugin name
shared. The unit tests exercise classloading and linkage behavior, but no install/replay test covers this namespace interaction. The already-reported absence of a production shared-layer consumer remains fenced by discussion r3978332587. - Scope and clarity: most edits are focused groundwork for a Hadoop-free FE; placing shared runtime bundles directly inside the legacy dynamic-plugin namespace is the one scope boundary that is not isolated.
- Concurrency: canonical-root
ConcurrentHashMap.computeIfAbsentgives one shared loader per physical root, and directory-plugin lifecycle mutation remains under its existing lock. No new thread entry, lock-order problem, deadlock, or query-time contention was found. - Lifecycle and static initialization: shared loaders intentionally live for the FE process; represented plugin failures close or discard their per-plugin loaders. Authenticator initialization/reset/close remains coherent and the new direct authenticator is stateless. No cross-TU/static-order concern applies.
- Configuration: no configuration item was added. The issue arises because the existing default
Config.plugin_dirresolves to the same packagedfe/pluginsparent used by this new directory. - Compatibility: auth-key literals and legacy re-export field descriptors are preserved, and supported connector SPI paths remain compatible. The newly occupied
sharedpath is an incompatible change to the previously accepted dynamic-plugin name space. No wire or storage-format change was found. - Parallel paths: connector, filesystem, authorization, legacy dynamic-plugin install/replay/uninstall, catalog authentication, HDFS resource/vault, and Maven packaging paths were traced. The inline issue is distinct from the existing no-consumer thread; no other PR-introduced defect survived.
- Conditional checks: archive extraction is correctly conditional on the zip, but
plugins/sharedis created before that condition, so the name collision occurs even when no archive exists. Other new conditions have clear failure semantics. - Test coverage: the new tests cover slash/dotted forbidden references, physical-root identity, jar precedence, attribute failures, and two linkage forms. They omit the legacy
name=sharedinstall/replay case and production shared-parent topology; the latter is already covered by the existing thread. - Test results: test assertions and changed expected behavior were inspected and are consistent; no result files changed. No local build or test was run because this review task explicitly forbids builds. Available exact-head dependency, license, and style checks were successful when inspected.
- Observability: represented plugin load failures have staged summaries and causes; no new metric is required for this startup-only work. The namespace collision currently surfaces only as an install failure, which the requested compatibility fix/test should make explicit.
- Transactions and persistence: no transaction or storage persistence format changes. Legacy plugin journal replay was traced and reinforces the same namespace collision.
- Data writes: no database data-write path or atomicity behavior changes.
- FE/BE variables: no new cross-process variable or Thrift propagation is introduced.
- Performance: shared-layer discovery is startup-only and the cache avoids duplicate loaders. The redundant shipped bundle cost is already reported in the existing no-consumer thread; no separate performance finding remains.
- Other issues and user focus: no additional user-provided focus was supplied. After two complete rounds, all normal and risk-focused reviewers returned
NO_NEW_VALUABLE_FINDINGS; every candidate is accepted, duplicate-fenced, or dismissed with evidence. Review status: complete for this head, changes requested.
| # 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" |
There was a problem hiding this comment.
[P2] Keep shared bundles out of the dynamic-plugin name namespace
Config.plugin_dir defaults to this same fe/plugins directory, while PluginInfo.readFromProperties() accepts every nonempty name, including shared. DynamicPluginLoader.movePlugin() maps the descriptor name directly to pluginDir/shared and refuses an existing target. Because this hunk creates that directory unconditionally—even when the Hadoop zip is absent—a valid legacy plugin named shared can no longer be installed on the default layout. Please place shared bundles outside Config.plugin_dir/<plugin-name>, or add an explicit backward-compatible reservation/migration rule with install/replay coverage.
There was a problem hiding this 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.
058ab76 to
dd9ca2c
Compare
|
run buildall |
1 similar comment
|
run buildall |
TPC-H: Total hot run time: 17112 ms |
TPC-DS: Total hot run time: 83307 ms |
ClickBench: Total hot run time: 14.76 s |
…s hidden Hadoop passengers
Groundwork for an FE kernel that ships no Hadoop classes at all — the end state
`ConnectorPluginManager` already names in its parent-first comment. Nothing is removed
from `fe/lib` here; this PR only makes the remaining coupling honest, so that the step
that does remove it cannot silently take unrelated libraries with it.
fe-common declared `hadoop-common` (provided) and `hadoop-aws` while not naming a single
Hadoop class — the module has zero `org.apache.hadoop` references, only two configuration-key
strings in `Config`. Dropping them exposed `Hll`, which uses
`org.apache.commons.codec.binary.StringUtils` and had been riding on hadoop-common's
commons-codec; that is now declared where it is used. `hadoop-aws` was compile scope, so it
also stops being re-exported to fe-type, fe-catalog, hive-udf and the BE's java-udf plugin,
none of which reference `org.apache.hadoop.fs.s3a` either. fe-core is unaffected: it declares
hadoop-aws itself.
The BE's java-udf plugin is the one consumer whose closure is checked by name
(tools/be-java-plugins/check_plugin_layout.py), and it excluded `hadoop-aws` from fe-common
explicitly; that exclusion is now dead and goes. In its place the plugin excludes
`commons-codec`: the layout check records that jar as deliberately absent from java-udf —
fe-common's only user of it, `Hll`, is on no BE path — and it used to be absent for free,
because hadoop-common's provided scope never re-exported it. With fe-common declaring it,
the plugin has to say so, or the jar walks in. Its plugin directory is unchanged as a result.
fe-core likewise names no Hadoop class in main, but two libraries it compiles against reach it
only as Hadoop transitives:
- metrics-core (`com.codahale.metrics`) via hadoop-auth — the entire metric layer is built on
it: MetricRepo, every MetricVisitor, HistogramMetric, CloudMetrics, SqlBlockRule.
- bcprov-jdk18on (`org.bouncycastle.util`) via hadoop-common — used by TableScanParams.
Both are now declared. They are already on the classpath at these versions, so no jar is added.
Finally, the three property names fe-core read from `AuthenticationConfig` — a fe-kerberos class
that imports `org.apache.hadoop.conf.Configuration` — move to a new hadoop-free holder in
fe-foundation, `HadoopAuthConfigKeys`. `AuthenticationConfig` re-exports every one of them, so
both spellings name the same property. The old copies were plain `public static String`, not
compile-time constants, so `HdfsStorageVault.PropertyKey` and `HdfsResource` loaded
`AuthenticationConfig` — and with it Hadoop's `Configuration` — to read a string; they no longer
do. fe-core keeps its fe-kerberos dependency for `ExecutionAuthenticator`, which is hadoop-free.
Verified: `build.sh --fe` builds all 83 modules, be-java-extensions included, with fe-core's
4476 main sources compiling and 0 Checkstyle violations anywhere. `dependency:list
-DincludeScope=runtime` for fe-core is byte-for-byte identical before and after — 395
artifacts — so `fe/lib` does not change. fe-common, fe-type and fe-catalog lose hadoop-aws and
its two transitives (wildfly-openssl, analyticsaccelerator-s3) and gain commons-codec. The
java-udf plugin directory keeps exactly its 27 jars, and check_plugin_layout.py passes on all
eight BE plugins.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XYs8EpL1D6bjufhYSt3mC4
Two comments in the FE kernel say why the 12.6 MB hive-exec:core jar sits in fe/lib, and both stop one step short of the truth. The pom's says the jar exists for CreateFunctionCommand, which validates a user JAVA_UDF jar by deriving its symbol class and so needs org.apache.hadoop.hive.ql.exec.UDF on the FE app classloader. ConnectorPluginManager's says that org.apache.hadoop.hive.* still comes from the plugins because "FE carries hive-exec:core, the plugins carry hive-metastore, and the class names do not intersect". For three of the four plugins that reference hive-exec classes the intersection really is empty, and the reason is the opposite of what that reads like: paimon, hive and iceberg bundle no hive-exec class at all, so every org.apache.hadoop.hive.ql.* reference they carry resolves to the kernel's copy. The fourth, hudi, bundles one subset - fe-connector-hudi unpacks hive-exec's ql/io/parquet/** (156 classes) into its own jar for the mapred/mapreduce hierarchy its pom explains - which the parent-first prefix shadows with the kernel's copy today, and which itself reaches a further 33 classes only the kernel has. Measured over the built plugin zips as referenced-minus-self-carried, cut against hive-exec:core's class list: paimon 15 classes (paimon-hive's PaimonStorageHandler implements ql.metadata.HiveStorageHandler, HiveCatalog .createView calls ql.metadata.Table.getEmptyTable, HiveUtils uses ql.io.sarg .ConvertAstToSearchArg), hudi 46 (hudi-hadoop-mr reaches 25 - HoodieCombineHive InputFormat's ql.io.HiveInputFormat, CombineHiveRecordReader and IOContextMap, the ORC branch of HoodieInputFormatUtils, the ql.plan descriptors - and the unpacked parquet subset 33 - ql.exec.Utilities, ql.io.HiveFileFormatUtils, ql.exec.vector.*, llap.LlapCacheAwareFs; so carrying that subset made the plugin depend on more of the jar, not less), hive and iceberg 3 each (the bundled Aliyun DLF metastore client calls ql.session.SessionState.get()). So narrowing fe/lib to be-java-extensions/hive-udf-shade, which carries 10 of those classes, compiles green and fails at runtime on those paths - the same shape as the hive-catalog-shade removal the pom comment already recounts. That is a prerequisite ordering worth writing down: the plugins have to become self-sufficient first. Note also that dropping the org.apache.hadoop. parent-first prefix would not change any of this, since it changes the delegation order and not the reachability - a plugin classloader that misses falls through to the app classloader either way. Comments only; no dependency, code or packaging change. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYs8EpL1D6bjufhYSt3mC4
…against Hadoop fe-kerberos is a 15-class module that splits cleanly in two: an 8-class Hadoop half (UserGroupInformation logins, AuthenticationConfig(Configuration), HadoopAuthenticator and friends) and a Hadoop-free remainder. fe-core used exactly one type out of it - ExecutionAuthenticator, which the module's own javadoc calls a migration bridge over the fe-foundation interface of the same name - and always with the passthrough implementation: no fe-core code path runs inside a Kerberos context. The Hadoop half is used only by the hive, hudi, iceberg and paimon connector plugins, each of which declares fe-kerberos itself. Depending on it from fe-core put those 8 classes in fe/lib, where they can never link, since the FE kernel does not ship the Hadoop they reference. Lazy resolution keeps that harmless today, and it leaves a compiles-green, fails-at-runtime path open: a future caller picking an overload whose signature mentions no Hadoop type gets past javac on the strength of fe-kerberos.jar being on the classpath, and only then hits NoClassDefFoundError on org/apache/hadoop/security/UserGroupInformation. So fe-core now takes ExecutionAuthenticator straight from fe-foundation (org.apache.doris.foundation.security) and drops the dependency. The two anonymous empty implementations become ExecutionAuthenticator.DIRECT, the constant fe-foundation provides for exactly this - the foundation interface leaves execute(Callable) abstract rather than defaulting it to a passthrough. The change is source-compatible for callers (both interfaces have the same execute(Callable)/execute(Runnable) shape, and the two call sites just run a lambda) and carries no metadata risk: ExternalCatalog.executionAuthenticator has no @SerializedName, so HiddenAnnotationExclusionStrategy keeps it out of persistence entirely. Also removes what the dependency was masking: ThreadPoolManager's three *WithPreAuth members and ExternalCatalog.threadPoolWithPreAuth, its shutdown and its getter. The field never had an assignment anywhere in the repo and the methods had no callers - dead since the connectors were pluginised. FeCoreHasNoHadoopClassesTest is the new guard, and the reason this is worth a commit rather than a cleanup. It scans fe-core's own compiled output and fails on a constant-pool reference to org/apache/hadoop/ or org/apache/doris/kerberos/, which is the form a dependency actually takes - it catches the reference no import reveals, such as an inherited supertype or a type that appears only in a descriptor. It matches the JVM internal spelling only, since ConnectorPluginManager and FileSystemPluginManager legitimately hold "org.apache.hadoop." as a parent-first classloader-policy string. Two assertions keep it from passing vacuously: it must scan more than a thousand classes, and it must find a positive control. Verified in both directions - green today (0 offenders in 6176 classes), red with the exact class named when a probe holding a new org.apache.hadoop.conf.Configuration() is compiled into fe-core. fe/lib loses fe-kerberos.jar; the four connector plugin zips still carry it, as they declare it directly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYs8EpL1D6bjufhYSt3mC4
…ing dependency from killing FE startup 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 bind to a single classloader per process. Hadoop is all three at once, which is why the FE kernel currently ships it and both plugin managers list org.apache.hadoop. as parent-first: giving each plugin its own copy is not the same thing duplicated, it is a behavior change. SharedLibraryLayer is where such a library goes instead of the kernel. It turns a root of bundle directories into one classloader that becomes the PARENT of every plugin classloader, so plugins reach it by the ordinary child-first fallback and a plugin carrying its own copy keeps using that one. The layout is the plugin directory convention - <root>/<bundle>/*.jar then <root>/<bundle>/lib/*.jar, bundles by name - which is also how a bundle ships a patched class that also exists in one of its dependency jars: patch in the bundle root, stock jar under lib/. The layer is memoized on the resolved root, which is the point of the class rather than an optimization: two layers over the same jars would mean two copies of every class in them, the exact situation it exists to prevent. A root that does not exist or holds no jar returns the parent unchanged, so a deployment that installs no bundle keeps the classloader graph it has today. A root that exists but cannot be read is raised instead, because degrading that to "nothing installed" turns a permissions mistake into a missing-class failure much later and somewhere else. Nothing calls resolve() yet - this commit adds the mechanism and its tests only, and no existing behavior changes. The second half is the gap that mechanism opens. Once a plugin can expect a dependency from a layer rather than from its own jars, "plugin installed, bundle not" becomes a reachable state, and the JVM reports it as NoClassDefFoundError - an Error, not a ReflectiveOperationException. DirectoryPluginRuntimeManager caught only the latter at the two points where it touches plugin bytecode, so the Error walked out of loadAll, out of initXxxPluginManager, and took FE startup with it. One uninstalled bundle must cost the plugins that need it, not the FE. Both catch sites now include LinkageError, matching what factory.name() and factory.description() already do a few lines below, and the failure message names the class that was missing and says where a dependency is expected to come from - a bundle that was never installed otherwise looks exactly like a broken plugin jar. The two failures land one step apart and both are covered: an absent supertype fails while the class is being resolved, an absent class reached from a static initializer fails at first initialization. The tests reproduce the real reachability rather than simulating it - the fabricated jar omits the dependency and the parent classloader refuses it - and both go red, with the NoClassDefFoundError escaping loadAll, if either catch is narrowed back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYs8EpL1D6bjufhYSt3mC4
…plugins Nine FE plugins need Hadoop and today each carries its own copy: 175MB across the plugin zips, nine independent FileSystem.CACHEs, nine UserGroupInformation logins, and a jindo native image that can only bind to one classloader per process. SharedLibraryLayer already knows how to load one such bundle for all of them; this is the bundle. fe-hadoop-runtime has no sources. It exists so the bundle has one dependency resolution and one artifact list, which is what turns "every plugin gets the same Hadoop" from a coincidence of nine poms agreeing into something checkable. It resolves what fe-filesystem-hdfs-base resolves - hadoop-client and hadoop-auth with the same exclusions, plus hadoop-common directly, since hadoop-client is the slim client POM and prunes com.jcraft:jsch, which the connector plugins ship today - and its versions come from the parent's dependencyManagement, so bundle and plugins cannot drift. Membership is an allowlist, not the resolved closure, and the two boundaries are in the assembly descriptor at length. What must be shared: classes that inherit across jars, plus the process-wide state that is what "two plugins talk to one cluster" actually means; hadoop-hdfs-client is in because hdfs:// resolves through META-INF/services rather than a fs.hdfs.impl entry, and the patched FileSystem scans those with its own loader. What must not be: anything reachable through fs.<scheme>.impl stays with the plugin that configures it, hadoop-aws above all, because the filesystem plugins take software.amazon.awssdk parent-first from fe/lib on purpose. General libraries - jackson, guava, netty, commons-* - stay out because fe/lib already has them at the version fe-core arbitrated and the layer falls back there; a second copy is how a shared layer starts answering with different versions than the kernel. The bundle produced here is exactly the set the nine plugins carry today: 28 jars in lib/, same filenames, therefore same versions, plus hadoop-deps.jar at the root, where the layer's root-before-lib ordering makes the Doris-patched FileSystem win over hadoop-common's. An allowlist fails silently when it is too narrow, so the module asserts the shape it produced: the patched jar is at the root, Hadoop made it into lib/, and nothing Doris-owned did. Nothing reads the bundle yet. build.sh unpacks it to fe/plugins/shared/hadoop/ and stops there - plugins/shared/ is not on the FE CLASSPATH and no plugin manager scans it, so this release ships the bundle alongside the kernel's Hadoop rather than instead of it. Verified by starting an FE on the built output: 14 filesystem plugins, 9 connector plugins and 2 authorization plugins load with zero failures, and not one log line mentions the new directory. fe-hadoop-runtime has to be named in build.sh's module list explicitly: nothing depends on it, so -am does not reach it, and a module that is never built leaves the deploy step silently unpacking whatever zip an older build left behind. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XYs8EpL1D6bjufhYSt3mC4
afc1c2e to
b9fc9a8
Compare
|
run buildall |
FE UT Coverage ReportIncrement line coverage |
…names, and carry the cause into the failure message
Review of the loader change in this branch found four defects in what a plugin
that fails to link leaves behind.
The cause-chain walk in missingClassName had no bound: the `t.getCause() == t`
guard is dead against java.lang.Throwable (getCause() already returns null for a
self-cause), and a two-node cycle is legal Java - a.initCause(b) after b was built
with cause a. A plugin constructor throwing such a chain spun FE startup forever
under the loader's lifecycle lock, before any port was open. The walk is now
identity-bounded, in both the advice and the new cause summary.
The walk read the first whitespace token of any NoClassDefFoundError or
ClassNotFoundException message as the class name. Plugin code wraps lookups in
sentences ("Cannot load driver class com.x.Y" advertised a missing class named
"Cannot"), and the JVM's own "a/B (wrong name: c/D)" form means the class WAS
found under the wrong jar path - a broken jar, for which the shared bundle is
exactly the wrong place to send the reader. Only a message that is one class name
in the binary or internal spelling is read as a miss; a sentence-shaped node is
skipped in favour of the JDK's own node below it; the wrong-name form gets its
own sentence.
For every LinkageError that is not a missing class - a static initializer
throwing IllegalStateException, a VerifyError - the failure message carried
nothing: ExceptionInInitializerError's message is null, so a consumer recording
the message alone (the authorization manager's rejection list, any log line that
consumed the throwable through a placeholder) recorded a bare class name. The
innermost cause is now appended to the LoadFailure message.
Both catch sites caught ReflectiveOperationException | LinkageError while their
comment appealed to factory.name()'s RuntimeException | LinkageError precedent.
asSubclass() throws ClassCastException for a service file naming a class that is
not a factory, and defineClass throws SecurityException for a signed package the
plugin also ships unsigned classes into; neither was caught, so the classloader
leaked and the exception reached FE startup. Both sites now catch RuntimeException
too, and the ClassCastException case names the factory type that was expected.
The comments that described <clinit> failures as always arriving as
ExceptionInInitializerError, and the "Could not initialize class" wording as
unreachable from loadAll, said what JDK 17 does not do: an Error thrown by an
initializer propagates unwrapped (JLS 12.4.2), and the second-attempt wording is
reachable through a parent-first class whose initializer failed under an earlier
plugin. Both are reworded; the catch shape they justify is unchanged.
Tests: a cyclic chain is walked once (under a timeout, so a regression hangs the
test rather than the suite); an initializer that throws a plain exception yields a
failure whose message carries the reason; a service file naming a non-factory
class is a load failure, not a throw; the sentence-shaped, trailing-dot and
wrong-name messages are negative controls, with the JDK node below a sentence
still found.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nagers' first calls into plugin code
Once a plugin that fails to link is skipped instead of taking FE startup down,
the record of why it was skipped is all the operator has. The three managers
that consume the loader's report logged each failure as
"message={}, cause={}" - four placeholders for four arguments, which log4j
renders with the throwable's toString() and no stack trace; for an
ExceptionInInitializerError that is the bare class name. The summary line
stayed at INFO. Each failure is now logged with three placeholders and the
throwable trailing, so the trace is printed (the shape the loader's README
prescribes and FileSystemPluginManager already used for its post-load call),
and the two shipped families - connector and filesystem - log the summary at
ERROR when any plugin failed, because an FE serving without a shipped plugin
is an FE serving degraded.
The loader's widened catch stops at the loader. The connector manager's first
calls into a loaded plugin - getType() and acceptedCreateTableEngineNames() -
ran with no guard, and the lineage manager ran factory.create() and
plugin.initialize() inside catch (Exception): a NoClassDefFoundError there,
one step after loadAll returned, still escaped to Env.initialize and exited
the FE. Both now catch RuntimeException | LinkageError, refuse that plugin and
release its classloader, mirroring the guard FileSystemPluginManager has on
sensitivePropertyKeys(). Built-in connector providers keep failing loudly: a
classpath provider that cannot link is a broken build, not a broken plugin
directory.
Tests: a directory connector provider whose getType() throws
NoClassDefFoundError is refused rather than thrown, and rethrown for a
built-in; a lineage plugin jar whose create() throws NoClassDefFoundError is
skipped by start() with no active plugin and no throw.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hout the plugin having loaded The aliases a filesystem plugin declares sensitive were masked only because each loaded plugin's sensitivePropertyKeys() was registered into DatasourcePrintableMap.SENSITIVE_KEY at startup. With a shipped plugin skipped at load - the state this branch makes survivable - the registration never ran, while every catalog, repository and vault created with that plugin's aliases stayed in metadata. SHOW CREATE CATALOG, SHOW CATALOG, the catalogs() TVF and the audit-log encryption of CREATE/LOAD/TVF/OUTFILE statements all mask through the same exact-key set, so cos.session_token, AZURE_CLIENT_SECRET, ozone.secret_key and sixteen more aliases were printed in clear. The static set now carries the 19 aliases the shipped plugins declare beyond the legacy union it already inlined, and exposes the static contents as an immutable BUILTIN_SENSITIVE_KEYS so a test can pin the providers' aliases to it without the registration that runs in the same JVM masking a gap: the test loads every FileSystemProvider on the test classpath and fails on any sensitive alias the static set does not contain. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ba673af made the two-argument BrokerDesc constructor rethrow the binding error when no broker name was given, so that OUTFILE on an absent provider reports the loaded providers instead of "Unknown broker name(null)". That message is part of the contract a regression test pins: export_p0/test_export_table_with_label_retry exports to a file:// path that routes nowhere and asserts exactly that text in the cancelled job's ErrorMsg, and cloud_p0 went red on it. The wording of that refusal is not this PR's concern; the fallback is restored as it was at the base, and the deserialisation-time tolerance in StorageDesc - the part of that commit the review asked for - stays. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/review |
|
run buildall |
There was a problem hiding this comment.
Static review status: complete for 28577df9f6df3446f7fd1d0c90032fb7d85c7733...7881f7e5b2f7225d18de71711eae29a05c99d426.
No new distinct inline finding survived the duplicate fence. The current head still contains issues already covered by existing threads: the shared Hadoop layer is not wired into the production plugin parent topology and occupies the shared plugin namespace; lineage plugin admission does not fully roll back factory/registry/instance state; and repository/broker recovery has the already-reported ALTER-repair, legacy-type, live ADD BROKER, and resumed-task failure gaps. I did not repost those issues. Earlier comments about physical-root identity, attribute-I/O propagation, class-initialization diagnostics, dotted reflective-name guarding, and the Bouncy Castle dependency are addressed on this head.
Critical-checkpoint conclusions:
- Goal and scenarios: the implementation decouples FE kernel dependencies, hardens per-plugin loading, stages a shared Hadoop bundle, and makes persisted storage/repository state tolerate missing providers. I traced normal startup, plugin failure, image/replay, restart, live ALTER/ADD BROKER, backup, restore, CloudRestore, load/export, and Azure-present/absent paths. The PR body is stale relative to this much broader 56-file change and does not describe these scenarios or their behavior changes.
- Minimal and focused change: the dependency cleanup, plugin runtime, storage routing, repository migration, lineage lifecycle, build packaging, and tests form a very large coupled patch. The cross-layer coupling is traceable, but this is not a narrowly isolated change.
- Concurrency and locks: shared-layer memoization, provider publication, repository-manager mutation, backup-job mutation, and the cached per-catalog filesystem lifecycle were checked. No new lock-order, publication-race, or heavyweight-under-lock issue was found beyond the existing lineage rollback thread.
- Lifecycle and initialization: class definition, static initialization, metadata probing, duplicate admission, discard, loader close, connector config, filesystem close, and lineage instance initialization/close were traced. Connector/filesystem rejection paths clean up; the distinct lineage partial-publication path is already threaded.
- Configuration and dynamic behavior: Hadoop config-directory injection, optional OBS/COS build profiles, Azure host-suffix configuration, plugin config fallback, and static/dynamic sensitive-key registration were checked. Loaded and absent Azure routing use the same probe view; no new drift or credential-masking gap was found.
- Compatibility and rolling upgrade: repository JSON keeps the legacy field while adding the descriptor migration path, transient adapters are rebound after load, and broker metadata is restored before backup repositories. The remaining restart/live-repair incompatibilities are exactly the existing repository threads; no additional format or FE protocol break was found.
- Parallel paths: built-in versus directory plugins, connector versus filesystem families, live versus replayed metadata, and backup/restore/CloudRestore/load/export consumers were compared. No distinct unfenced path remained.
- Special conditions and comments: null/empty properties, absent or throwing providers, symlinks, unreadable attributes, repeated builds, duplicate names, locale-sensitive folding, no-backend connectivity checks, and initialization-failure cause chains were reviewed. Current comments generally explain the non-obvious invariants; no new comment/code mismatch rose to a finding.
- Tests: the patch adds focused unit coverage for loader/linkage failures, shared-layer identity and ordering, repository persistence/recovery, Azure parity, masking, connector context, and backend-property propagation. The negative cases missing from those tests map to existing review threads. Per workflow contract I ran no local build or tests. At the submission gate, GitHub reported COMPILE, BE UT, Cloud UT, coverage, dependency/license, and CheckStyle successful; FE UT and regression/performance suites were still pending.
- Observability and errors: plugin failures retain stage/cause and stack traces, repository unavailability is surfaced, and job-loop boundaries were traced. No new swallowed error or misleading metric/log path was found beyond the existing broker task-construction issue.
- Persistence and failover: image module order, Gson post-processing, journal replay, checkpoint serialization, resumed jobs, and corrective ALTER were inspected. Every substantiated defect here is already fenced by the four existing repository/broker threads.
- Data writes and atomicity: no table/storage-engine write path changes. Repository create/alter validation, manager publication, journaling, and remote-operation status propagation were checked; no new partial metadata commit was found.
- FE/BE propagation: storage type, normalized locations, backend properties, broker addresses, and vended/static credentials were followed from FE binding through backup/restore/load/export task construction. The only surviving exception boundary is already reported on
FileSystemDescriptor. - Performance: cached normalized locations and per-catalog filesystems avoid repeated binding; shared-layer scanning is process-lifetime memoized. No new unbounded hot-path work or memory-retention issue was found apart from the already-threaded lineage loader retention.
- Optimizer plan trees: not applicable; the Nereids changes are repository display/resource-analysis commands rather than optimizer rewrites or plan-node generation.
- Other: dependency assembly, archive cleanup/precedence, public bridge types, removed pre-auth APIs, and all 56 changed paths were included in the final sweep. Current/base searches found no in-tree consumer of the removed APIs and no distinct runtime linkage regression.
User focus: no additional focus was supplied, so the review covered the full PR without a narrower override.
This is a summary-only review, not an approval of the existing unresolved threads. Review evidence is static except for the live PR SHA and CI-status checks noted above.
…air an unbound repository, and roll back a refused lineage plugin Follow-ups to the review-bot findings on the previous head. A typed legacy record whose provider is loaded but threw or rejected the properties during migration, with a broker of the same name registered, still took the broker fallback: the rule only recognised an absent provider as the explanation, so a provider failure plus a same-name broker persisted the record as BROKER - the original defect through one more door. A name that is a storage type's (a shipped provider other than BROKER, which is a family a broker may well be named after) is now never read as a broker's: the record is kept with the reason, absent or failed. Only a name that is no storage type's and is a registered broker's is a broker record. ALTER REPOSITORY refused every unavailable repository, including one whose descriptor merely did not bind at load because a formerly accepted property is now rejected - the case ALTER exists to repair. It now refuses only a record with no descriptor to merge into, binds the merged properties itself and reports the binding's reason as the DDL error when they still do not. A broker repository binds per call and nothing checks its provider at load, so a resumed backup or restore passed getBrokerAddress() and threw from StorageAdapter.ofBroker() while constructing its task. getBrokerAddress() binds the broker descriptor and answers with a Status instead. A lineage plugin refused at create()/initialize() left its factory in the map (retaining the discarded classloader) and an instance whose initialize() had failed unclosed. Both go with the plugin now; the inventory row stays, since the registry has no removal and a row for a plugin that was found but refused is what an operator reading information_schema.extensions after the WARN needs. A legacy record whose broker is not registered says to restart after ADD BROKER: the migration is retried at load only. Tests: a same-name broker with a throwing provider keeps the typed record; a repository unbound at load is repaired by ALTER and a still-unbindable correction is refused with the reason; a lineage plugin whose initialize() throws is closed and its factory dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/review |
|
run buildall |
There was a problem hiding this comment.
Requesting changes on exact range 28577df9f6df3446f7fd1d0c90032fb7d85c7733...c1fc199b3a4ef7760ce1dd9fb71ee2888ca6d5e4. Two distinct PR-local P2 findings are inline. Existing rendered threads, raw review comments, and prior summaries were treated as hard duplicate fences.
Critical-checkpoint conclusions:
- Goal and proof: the dependency isolation, plugin-failure containment, and repository migration work is broadly coherent, but late filesystem linkage is not contained at repository I/O and the Azure-absent routing branch does not honor an explicitly empty live suffix list. Focused tests cover many degraded-provider cases but omit these two triggers.
- Scope and clarity: all 56 authoritative changed paths match the authoritative diff. The build, loader, authentication, storage, repository, and persistence edits are related, but this is a large cross-layer change rather than a small local refactor.
- Concurrency and locks: plugin admission follows the existing loader lifecycle synchronization; repository maps retain their concurrent-map/manager-lock protection; repository ping and backup daemons can overlap live DDL. No new race, lock-order inversion, deadlock, or heavy I/O under a newly introduced lock was found.
- Error handling: M-001 violates the new nonthrowing repository boundary:
NoClassDefFoundErrorescapes the localStatus/errMsgpaths and is only logged by the outer daemon. Other candidate exception paths were either already threaded or converted to their normal status boundary. - Memory safety: no BE/native allocation path changes. Loader/factory ownership and close/discard paths were traced; the one substantiated lineage retention/rollback issue is already covered by existing discussion
r4018143606. - Lifecycle and initialization: plugin discovery, definition, static initialization, admission, image load, replay, checkpoint, provider loss/recovery, and first filesystem use were traced. First-use JVM linkage is the uncovered lifecycle transition in M-001; other repository recovery gaps are already fenced by existing threads.
- Configuration: no new config item is added. The existing
azure_blob_host_suffixesarray is mutable and read dynamically, but its valid empty state is lost in M-002; nonempty overrides and explicitprovider=azureretain their intended behavior. - Compatibility and rolling behavior: the foundation/Kerberos bridge preserves in-tree type/property compatibility, and legacy repository metadata is retained when migration cannot be decided. No additional wire, storage-format, edit-log, or rolling-upgrade defect survived review beyond the existing repository threads.
- Parallel paths: connector/filesystem/lineage loaders; direct and BROKER repositories; backup/restore/cloud restore/load/export; live/replayed ALTER; and loaded/absent Azure routes were compared. Parallel late direct-filesystem failures collapse into M-001, while broker task construction remains fenced by
r4018143626. - Conditional checks: provider presence, storage-type versus broker classification, unavailable-repository guards, Azure guess fallback, and null/empty properties were checked. M-002 is the one new condition that conflates “no probe context” with “configured empty override.”
- Test coverage: added unit tests cover absent/throwing providers, repository migration/replay, shared-loader identity, masking, Azure routing, and plugin admission. Missing negative cases are a filesystem that links only on first
exists/listand a cleared Azure suffix config with the provider loaded and absent. - Test results: no local build or test was run because this review contract forbids it; conclusions are static-only. Live exact-head CI currently reports COMPILE, BE UT, Cloud UT, CheckStyle, license, and dependency review successful; FE UT and regression jobs remain pending, and
check_coverage_fecurrently reports failure. These are CI results, not independent execution. - Observability: plugin-stage logging and stored repository reasons generally improve diagnosis. M-001 leaves repository status null/stale and exposes the cause only in the outer daemon log; no additional metric/logging defect survived.
- Persistence and transactions: repository image migration, checkpoint precedence, manager replacement, edit-log replay, corrective ALTER, and resumed jobs were traced. No user transaction-visibility path changes; the remaining metadata/recovery defects outside these two findings are already covered by existing discussions.
- Data writes and crash behavior: no table-data write, MoW, or delete-bitmap path changes. Repository metadata and remote-operation boundaries were checked; no additional partial commit or crash inconsistency was substantiated.
- FE/BE propagation: no new Thrift/session variable is introduced. Storage type, normalized location, backend properties, broker address, and credentials were followed through backup/restore/load/export task construction; no new distinct propagation issue survived.
- BE null/nullable handling: not applicable; this PR changes FE Java/build code and no BE column path.
- Performance: plugin scans, map copies, binding, and shared-layer memoization are startup/control-plane work; no material hot-path CPU, memory, or I/O regression was found. The unused bundle cost is already covered by existing comment
3978332587. - Other issues and focus: no additional user focus was supplied. Two bounded rounds converged; both normal reviewers and the separate risk reviewer returned
NO_NEW_VALUABLE_FINDINGSin the final round. Review status: complete for this exact head, static-only, changes requested.
| + ": Invalid path. " + path + ", error: " + e.getMessage(); | ||
| return false; | ||
| } catch (IOException e) { | ||
| } catch (IOException | RuntimeException e) { |
There was a problem hiding this comment.
[P2] Contain late filesystem linkage failures inside the repository boundary
A provider can bind and construct its filesystem successfully, then resolve a missing class only when exists() or list() first executes. This catch now contains RuntimeException but still lets NoClassDefFoundError escape. RepositoryMgr's new outer catch only logs it, so ping() leaves errMsg null or stale; listSnapshots() likewise bypasses its Status path and live BACKUP submission throws instead of reporting repository unavailability. Please contain LinkageError at the repository I/O boundary (including list paths), record or return the normal error status, and test a filesystem whose first I/O call fails to link.
| // Azure-shaped map must be refused here rather than fall through to type=s3: the resource | ||
| // type is persisted and ALTER RESOURCE cannot change it, so an S3Resource created while | ||
| // the plugin was absent would stay one after the plugin is repaired. | ||
| if (AzureBlobEndpointSignals.guessIsAzure(FileSystemPluginManager.withProbeContext(properties))) { |
There was a problem hiding this comment.
[P2] Preserve an explicitly empty Azure suffix whitelist
When Config.azure_blob_host_suffixes is cleared at runtime, withProbeContext() omits the marker, while AzureBlobEndpointSignals treats an absent marker as “use built-in defaults.” This new Azure-plugin-absent fallback therefore reclassifies *.blob.core.windows.net as Azure and rejects type=s3 even though the live whitelist is empty. Please carry an explicit empty override through the probe context, distinguish it from no context, and cover both loaded- and absent-Azure-provider paths.
…ll inside the repository A provider can bind and construct its filesystem and only link a missing class when exists() or list() first runs - the plugin-absent case one step later than the binding. The repository's I/O paths contained IOException (and ping() a RuntimeException) there but let the LinkageError through: ping() left errMsg stale so SHOW REPOSITORIES showed the repository healthy, and listSnapshots(), upload() and download() threw past their Status instead of reporting it, into the job loop. Each of those boundaries now returns or records the error like an IOException. Test: a filesystem whose exists() and list() throw NoClassDefFoundError makes ping() false with the reason in errMsg and listSnapshots() an error Status, without a throw. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/review |
|
run buildall |
There was a problem hiding this comment.
Request changes: three distinct plugin-boundary correctness gaps remain on exact head 05b472131ef889d8cefbaa0846e3c683f8ea99d7.
- A malformed nested
ServiceLoaderlookup can still escape the new directory-plugin guards asServiceConfigurationError, abort the family load, and bypass classloader cleanup. - Connector engine names remain a plugin-owned
Settraversed twice outside the failure guard, so traversal failure can abort later plugins and leave partial claims. - Post-admission filesystem probing/binding can raise the same service-configuration error during
StorageDescreplay and abort image load or follower journal replay instead of taking the documented retry-at-use path.
Critical checkpoints: correctness, lifecycle, persistence/replay, cleanup ownership, and negative-path coverage are blocked by the inline findings. Startup serialization, repository/job locking, copy-on-write provider publication, compatibility of the foundation/kerberos split, dependency reachability, Azure routing outside the already-reported explicit-empty case, legacy/new repository formats, FE/BE property propagation, observability for contained failures, and performance were traced with no additional unfenced issue. The change is broad but otherwise internally coherent and readable. Existing review threads were treated as hard duplicate fences.
Tests: static review only as required by the review task; I did not run builds or tests. The live CI snapshot at submission has check_coverage_fe failed and nine checks pending.
Focus: no additional user-provided review focus was supplied.
| factory = factoryClass.getDeclaredConstructor().newInstance(); | ||
| } catch (ReflectiveOperationException e) { | ||
| } catch (ReflectiveOperationException | RuntimeException | LinkageError e) { | ||
| // newInstance() is where the factory class is first initialized. A static initializer |
There was a problem hiding this comment.
[P2] Contain ServiceLoader configuration failures per plugin
ServiceConfigurationError is an Error, but not a LinkageError, so a factory initializer or name()/description() callback that performs a malformed nested ServiceLoader lookup still escapes this new catch. loadAll() catches only PluginLoadException, which means one bad directory plugin aborts the rest of the family load and this runtime classloader is never closed. The built-in connector/lineage paths already treat this standard provider failure as rejectable. Please include ServiceConfigurationError in every new external-plugin guard (generic factory/metadata plus family admission/init/cleanup), convert it to the normal per-plugin failure/cleanup path, and cover a malformed nested service lookup.
| try { | ||
| type = provider.getType(); | ||
| engineNames = provider.acceptedCreateTableEngineNames(); | ||
| } catch (RuntimeException | LinkageError e) { |
There was a problem hiding this comment.
[P2] Snapshot plugin-owned engine names inside the failure guard
This guard only calls acceptedCreateTableEngineNames(); the returned plugin-owned Set is first traversed later in createTableEngineNameProblem() and then traversed again after claimedTypes is mutated. A lazy/custom set whose iterator links a missing optional class (or otherwise throws) therefore escapes registerDiscovered(), aborts the remaining connector plugins, and leaves this handle undiscarded; a second-pass failure also leaves the type name claimed. Please materialize the set into a host-owned snapshot inside this same guard, validate/publish that snapshot, and test an iterator-time failure.
| try { | ||
| initStorageAdapter(); | ||
| } catch (RuntimeException | LinkageError e) { | ||
| LOG.warn("Storage descriptor (name={}, type={}) could not bind its filesystem provider at" |
There was a problem hiding this comment.
[P2] Contain service-configuration failures during replay binding
ServiceConfigurationError is an Error, not a LinkageError, so an admitted filesystem provider that performs a property-dependent nested ServiceLoader lookup from supportsExplicit(), supportsGuess(), or bind() still escapes this new guard. Because gsonPostProcess() runs while persisted load/export jobs are restored, that aborts image load or follower journal replay instead of leaving the descriptor unbound for the documented retry-at-use path. This is post-admission, so fixing the directory-loader boundary does not cover it. Please catch ServiceConfigurationError explicitly at these provider-backed persistence boundaries and add a StorageDesc round-trip case whose provider throws it while probing or binding.
TPC-H: Total hot run time: 16855 ms |
TPC-DS: Total hot run time: 83563 ms |
ClickBench: Total hot run time: 14.82 s |
FE UT Coverage ReportIncrement line coverage |
… and walk a plugin's engine-name set inside the guard ServiceConfigurationError is an Error but not a LinkageError: a plugin whose static initializer, name(), probe or bind runs a nested ServiceLoader lookup over one of its own malformed service files - its driver discovery - threw it through every guard this branch added, which caught RuntimeException and LinkageError only. In the loader that aborted the rest of the family load and leaked the classloader; in StorageDesc.gsonPostProcess it took image load or a follower's journal replay down; in the family managers, the repository binds and I/O boundaries and the job and ping loops it escaped the same way. Every such guard now names ServiceConfigurationError too. The connector manager's first-call guard covered calling acceptedCreateTableEngineNames() but not walking its answer: the plugin-owned set was first traversed in the problem checks and again while claiming, both outside the guard, so a lazy set whose iterator links a missing class threw out of registerDiscovered() - after the type name had been claimed on the second pass. The set is copied into a host-owned snapshot inside the guard, and the checks and claims walk the copy. Tests: a plugin whose initializer runs a nested lookup over a service file naming a class that does not exist is one LoadFailure whose cause is the ServiceConfigurationError; a provider whose probe throws it leaves a persisted load job's descriptor unbound at replay instead of failing the replay; a connector provider whose engine-name set throws when walked is refused with no type claimed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/review |
|
run buildall |
There was a problem hiding this comment.
Request changes: two new correctness issues remain on exact head b273abf0be9bc1088ba4336b4234384846665a57: legacy broker migration can persist the wrong storage identity, and filesystem plugin admission can publish state before validating plugin-owned sensitive-key metadata.
Critical checkpoints:
- Goal and proof: The dependency/classloader decoupling and failure-containment direction is implemented and has substantial unit coverage, but the two inline cases show the end-to-end goal is not yet met. The missing checkpoint/reload broker-shape tests and lazy-set/content tests are required.
- Scope/focus: The 58-file change is broader than the PR description (dependency cleanup plus plugin/repository lifecycle fixes), so it is not narrowly scoped; every authoritative path was reviewed. No additional user focus was supplied.
- Concurrency: Plugin loading is startup-oriented and provider lists use copy-on-write; repository maps use their existing manager lock. No new deadlock/race was substantiated, but the filesystem issue is non-transactional publication even without a race.
- Lifecycle: Loader handle discard/close, registry/provider publication, lineage initialization/close, and resumed backup/restore paths were traced. The filesystem issue leaves retained/partially published state.
- Configuration: Dynamic Azure suffix routing and loaded/absent-provider parity were checked; the known explicit-empty issue is already covered by an existing thread. No other new configuration issue was found.
- Compatibility: Dependency/authenticator/API and old/new plugin classloader combinations were checked. No additional supported API/rolling issue was found, but the broker migration issue breaks persisted legacy repository compatibility.
- Parallel paths and conditions: Built-in and directory filesystem admission were both checked and share the sensitive-key issue. The storage-type-name/broker/provider decision table is covered by the migration issue.
- Tests and results: Existing tests cover many negative loader/provider paths but omit the two exact cases above. Per the task contract this was static-only: no build, unit test, or regression test was run, and no generated result was modified.
- Observability: New logs improve plugin/repository diagnosis; no separate metrics gap was substantiated. Logging does not repair partial publication or a wrong persisted identity.
- Persistence/failover: Edit-log replay, image load/checkpoint, ALTER replay, heterogeneous FE/provider availability, and master failover were traced. The migration issue is blocking because checkpoint/reload preserves the wrong or unrecoverable identity.
- Writes/atomicity: No BE data-write protocol changes were introduced. Plugin admission is not atomic in the filesystem issue, and metadata identity persistence is incorrect in the migration issue.
- FE/BE propagation: No new thrift/config variable requires additional propagation; backend repository property/task construction paths were traced.
- Performance: Changes are predominantly startup/load-time; no new hot-path regression or material optimization gap was found.
- Other issues: Existing review threads remain the duplicate fence for shared-runtime wiring, loader/linkage containment, Azure empty override, lineage rollback, and related repository recovery cases.
Review status: complete after three bounded rounds; all final-round reviewers returned NO_NEW_VALUABLE_FINDINGS beyond the two accepted comments.
| // edit-log replay: LinkageError included, so a half-installed plugin costs this | ||
| // repository, not the FE - the same catch CatalogFactory uses for catalogs. | ||
| try { | ||
| StorageAdapter storageAdapter = StorageAdapter.of(props); |
There was a problem hiding this comment.
[P1] Preserve broker identity before routing legacy properties
This routes prop before consulting the registered broker name. A real legacy WITH BROKER broker_0 HDFS repository commonly stores fs.defaultFS/hadoop.username; the HDFS provider therefore binds here, and the next checkpoint permanently records HDFS instead of BROKER, so later backup I/O bypasses broker_0. The catch-side name heuristic has the inverse edge too: a legal broker actually named HDFS is left descriptor-less and cannot be repaired by ALTER. These are distinct from the existing typed-provider-failure thread because the provider succeeds and the old record genuinely is BROKER. Please recognize ordinary registered brokers before typed routing and retain/add an explicit discriminator or repair path for ambiguous names, with checkpoint/reload tests for broker HDFS/S3 passthrough properties.
| try { | ||
| sensitiveKeys = provider.sensitivePropertyKeys(); | ||
| } catch (RuntimeException | LinkageError e) { | ||
| } catch (RuntimeException | LinkageError | ServiceConfigurationError e) { |
There was a problem hiding this comment.
[P2] Materialize the sensitive-key set inside this guard
The guarded call can return a plugin-owned lazy/custom Set; its iterator is first consumed later by registerSensitiveKeys() via the case-insensitive addAll. At that point providers.add(provider) has already made the directory provider routable, so an iterator-time failure (or a null/invalid element) aborts the family load with the provider active, its aliases partially/not masked, no inventory row, and later successful handles skipped. loadBuiltins() has the same root cause in the opposite publication order: it writes the inventory row before traversing the set and does not roll it back on failure. This is distinct from the connector engine-name thread because this path controls filesystem admission and secret masking. Please materialize and validate a host-owned set inside the guard before any publication in both paths, discard on failure, and test iterator/content failures.
FE UT Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16808 ms |
…and walk a provider's sensitive keys before publishing it The legacy migration routed the record's properties first and consulted its name only when no provider claimed them. A WITH BROKER repository commonly stores fs.defaultFS and hadoop.username, which the HDFS provider claims, so such a record was migrated as HDFS, the next checkpoint made that permanent, and every later backup bypassed the broker it was created on. The record's name is the one identity it carries - a storage type for a typed record, the broker for a broker record - and now decides: a storage type's name binds that type (or is kept with the reason, which for a broker that happens to be named after a storage type says to DROP and re-CREATE); a registered broker's name is a broker record, whatever its properties route to; only a name that is neither - one from before the type names, or a broker since dropped - is routed as before. FileSystemPluginManager guarded the call to sensitivePropertyKeys() but not the walk over its answer, which happened in the masking set's addAll after the provider was already routable (and, for a built-in, after its inventory row): a plugin-owned lazy set failing when iterated, or a null alias failing the case-insensitive comparator, aborted the family load with the provider active and unmasked. The set is copied and checked inside the guard, before anything is published, on all three admission paths. Tests: a legacy broker record with HDFS-shaped properties stays a broker repository through a checkpoint round trip; a provider whose sensitive-key set fails when walked, or carries a null alias, is refused before it is published. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
/review |
|
run buildall |
There was a problem hiding this comment.
Request changes on exact head 7467c22861a7bd6a9330bd33c9597acfe05fb3d9.
I found three distinct exact-head defects, reported inline:
- Typed legacy repository migration recognizes the saved provider name, then ignores it and priority-routes the properties, so a valid AZURE record can be checkpointed permanently as S3.
- Backend task-property conversion is brought inside the job
Statusboundary only for BROKER; the parallel non-BROKER path can still throw from upload/restore task construction and retry unchanged until timeout. - The new continue-after-plugin-rejection path can replay metadata without an out-of-tree provider's process-local sensitive aliases, exposing those stored credentials through catalog SHOW surfaces.
Critical-checkpoint conclusions:
- Goal and proof: the exact head is a broad FE filesystem-SPI, plugin-failure, Hadoop-runtime, and repository-migration change. It makes substantial progress, but the three transitions above mean the migration/degraded-operation goal is not yet correct. The changed unit tests do not cover those transitions.
- Scope and focus: 59 files are changed across build/deployment, FE core, extension loading, filesystem SPI/providers, persistence, and tests. That is materially broader than the current PR description/checklist, which should be refreshed before merge.
- Concurrency: the new process-wide provider/registry state uses the existing startup ordering and copy-on-write provider collections; I found no new distinct lock, race, or deadlock issue after fencing the existing plugin-publication threads.
- Lifecycle: startup admission, image replay, checkpoint, ALTER, resumed jobs, and plugin rejection were traced end to end. The restart and resumed-job gaps are covered by the inline findings; other substantiated lifecycle defects are already covered by existing review threads.
- Configuration: the loaded/absent Azure routing paths share the new probe context for nonempty suffix settings. The explicit-empty setting defect is already covered by an existing thread; no additional distinct configuration issue survived review.
- Compatibility: legacy repository metadata and FE-to-BE task properties are compatibility-sensitive. The typed-record rewrite and non-BROKER projection gaps are reported inline. I found no additional distinct public API/runtime-closure issue beyond existing threads.
- Parallel paths and conditions: BROKER versus non-BROKER, built-in versus out-of-tree providers, admitted versus rejected plugins, loaded versus absent Azure, and constructor versus replay paths were compared. The asymmetric non-BROKER guard and rejected-plugin masking path are reported inline.
- Tests and results: 28 changed/new test-source files exercise many local cases, but miss the three concrete cross-restart/task-construction transitions above. This review was static only; I did not run builds or tests. At submission, BE UT, Cloud UT, coverage, CheckStyle, dependency review, license, secret, large-file, and formatter checks are green; FE UT, compile, performance, and the automated review contexts are still pending.
- Observability: plugin load summaries and repository unavailability messages are generally improved and include useful causes/identifiers. I found no separate observability blocker.
- Persistence, transactions, and writes: repository descriptor creation, checkpoint preference, image/edit-log replay, ALTER replacement, and active-job refresh were traced. The permanent wrong-type checkpoint is the blocking persistence defect; no BE data-write transaction path is changed here.
- FE/BE propagation: storage type and backend properties reach backup, restore, and cloud-restore task construction. The missing non-BROKER validation boundary is reported inline; no other new transmitted field was introduced.
- Performance: I found no substantiated new hot-path complexity, allocation, or I/O regression distinct from the existing review context.
- Other issues: existing inline threads were treated as hard duplicate fences and are not repeated here. There was no additional user-provided review focus.
This is a complete static review of the supplied exact-head diff after convergence; it is not build or test validation.
| // reason; a registered broker's name is a broker record; anything else - a name from before | ||
| // the type names, or a broker since dropped - is routed as it always was. A kept record is | ||
| // retried at the next start, and every use reports the reason until then. Every bind runs | ||
| // plugin code at image load and edit-log replay, so a LinkageError or a plugin's own |
There was a problem hiding this comment.
[P1] Bind typed legacy records through their saved provider
This branch treats fsName as the authoritative storage type but then ignores it by calling generic priority routing. For example, a valid legacy AZURE record that also carries a stale fs.s3.support=true flag is claimed by S3 first; the next line builds an S3 descriptor, and the next checkpoint permanently makes backup/restore use S3 normalization and BE properties. This is distinct from the broker-identity thread because the typed branch is already selected and every bind succeeds. Please use StorageAdapter.ofProvider(fsName, props) here and cover checkpoint/reload with properties that another provider also claims.
| if (unavailableReason != null) { | ||
| // The first repository call every backup and restore job makes before it reads the | ||
| // descriptor's properties: a job on an unusable repository fails here with a Status and is | ||
| // cancelled, instead of throwing out of run() on every tick. |
There was a problem hiding this comment.
[P2] Validate task properties for non-broker repositories too
This guard is reached only after the non-BROKER early return, but backend-property projection is a separate provider callback that bind/create/ping never exercises. A non-BROKER provider can therefore serve FE I/O and still return no toBackendProperties() value or link-fail there; upload, restore, and cloud-restore then throw while constructing their BE tasks, and the daemon only logs/retries the unchanged job until timeout. ALTER can likewise journal the new repository before active-task refresh throws. Please validate/cache this projection for every repository type inside the Status boundary (and before committing ALTER), then pass that validated map to task construction.
| // The aliases the shipped filesystem plugins declare sensitive beyond the legacy union above. | ||
| // Masking must not depend on a plugin having loaded: a shipped plugin that fails to load is | ||
| // skipped and the FE serves on, while every catalog, repository and vault created with its | ||
| // aliases stays in metadata and is printed by SHOW CREATE CATALOG, SHOW CATALOG, the |
There was a problem hiding this comment.
[P1] Preserve custom sensitive aliases when a plugin is rejected
The new degraded-startup path can now reject an out-of-tree provider and continue into image replay, but that provider's sensitivePropertyKeys() snapshot lived only in the previous JVM. This fallback restores shipped aliases only, so a catalog created with a custom alias such as acme.auth_blob is replayed after the plugin link failure and SHOW/SHOW CREATE prints the credential verbatim to users with catalog SHOW privilege. This is distinct from the lazy-set thread: admission succeeded in the old process and the provider is wholly absent in the new one. Please persist a host-owned alias snapshot independently of executable provider admission and restore it before printable metadata becomes available, with a successful-create then rejected-on-restart test.
FE Regression Coverage ReportIncrement line coverage |
TPC-H: Total hot run time: 16968 ms |
TPC-DS: Total hot run time: 81720 ms |
ClickBench: Total hot run time: 14.69 s |
2 participants
What problem does this PR solve?
Issue Number: None
Related PR: #66770
Problem Summary:
Groundwork for an FE kernel that ships no Hadoop classes at all — the end state
ConnectorPluginManageralready names in its parent-first comment ("the intended end state isan FE kernel with no hadoop classes at all, every plugin bringing its own"), and the one the
authorization plugin family already reached:
org.apache.hadoop.is child-first there and theRanger plugins bundle their own Hadoop.
Nothing is removed from
fe/libhere. This PR only makes the remaining coupling honest, sothat the step which does remove it cannot silently take unrelated libraries with it.
1. fe-common stops depending on Hadoop
fe-common declared
hadoop-common(provided) andhadoop-awswhile not naming a single Hadoopclass — the module has zero
org.apache.hadoopreferences in source, only two configuration-keystrings in
Config.Dropping them exposed one real user:
Hllusesorg.apache.commons.codec.binary.StringUtilsandhad been riding on hadoop-common's
commons-codec. That is now declared where it is used (theversion is already managed in
fe/pom.xml, and fe-core has long declared the same artifact).hadoop-awswas compile scope, so it also stops being re-exported to fe-type, fe-catalog,hive-udf and the BE's java-udf plugin — none of which reference
org.apache.hadoop.fs.s3aeither. There is no
import org.apache.hadoop.fs.s3a...anywhere in the repository; every mentionis a configuration-value string such as
"org.apache.hadoop.fs.s3a.S3AFileSystem", produced byfilesystem plugins that declare
hadoop-awsthemselves. fe-core is unaffected: it declareshadoop-awsdirectly.The java-udf plugin is the one consumer whose closure is checked by name
(
tools/be-java-plugins/check_plugin_layout.py), and it excludedhadoop-awsfrom fe-commonexplicitly; that exclusion is now dead and goes. In its place the plugin excludes
commons-codec:the layout check records that jar as deliberately absent from java-udf — fe-common's only user of
it,
Hll, is on no BE path — and it used to be absent for free, because hadoop-common's providedscope never re-exported it. With fe-common declaring it, the plugin has to say so, or the jar
walks in. Its plugin directory is unchanged as a result.
2. fe-core's hidden Hadoop passengers are declared
fe-core likewise names no Hadoop class in
src/main— its sixorg.apache.hadoopoccurrences arecomments and the two parent-first prefix lists. But two libraries it compiles against directly
reach it only as Hadoop transitives:
io.dropwizard.metrics:metrics-core(com.codahale.metrics)hadoop-authMetricRepo, everyMetricVisitor,HistogramMetric,CloudMetrics,SqlBlockRuleorg.bouncycastle:bcprov-jdk18on(org.bouncycastle.util)hadoop-commonTableScanParamsBoth are now declared in fe-core. They are already on this classpath at these versions, so no jar
is added to
fe/lib; the point is that the FE metric layer should not be a passenger of adependency that says nothing about it.
3. The Hadoop auth property names move to fe-foundation
fe-core read three property names from
org.apache.doris.kerberos.AuthenticationConfig, a classthat imports
org.apache.hadoop.conf.Configuration. The names are plain strings; the code thatturns them into a
UserGroupInformationis what needs Hadoop.They move to a new hadoop-free holder,
org.apache.doris.foundation.security.HadoopAuthConfigKeys.AuthenticationConfigre-exports every constant declared there, so both spellings name the sameproperty and no existing caller changes meaning.
This is not only a compile-time tidy-up. The old copies were
public static String— notcompile-time constants — so reading one emitted a
getstatic, and initialisingHdfsStorageVault.PropertyKeyor runningHdfsResource.generateHdfsParamloadedAuthenticationConfig, and with it Hadoop'sConfiguration, just to obtain a string. They nolonger do. fe-core keeps its fe-kerberos dependency for
ExecutionAuthenticator, which ishadoop-free.
Release note
None
Check List (For Author)
Test
Verification performed:
build.sh --fe— all 83 modules build, be-java-extensions included; fe-core's 4476 mainsources compile with 0 Checkstyle violations in every module.
tools/be-java-plugins/check_plugin_layout.pypasses on all eight BE plugins — the onefe-common consumer left on the BE side loses nothing it uses.
mvn -pl fe-core dependency:list -DincludeScope=runtimeis byte-for-byte identical beforeand after this change — 395 artifacts — so
fe/libdoes not change. fe-common, fe-type andfe-catalog lose
hadoop-awsand its two transitives (wildfly-openssl,analyticsaccelerator-s3) and gaincommons-codec.as transitives, at the same versions.
Behavior changed:
Does this need documentation?
Check List (For Reviewer who merge this PR)
🤖 Generated with Claude Code
https://claude.ai/code/session_01XYs8EpL1D6bjufhYSt3mC4