Repository navigation
Support taking external runtime handle - #255
Merged
Merged
Conversation
mixermt
added a commit
to mixermt/iceberg-rust
that referenced
this pull request
Oct 9, 2026
opendal's hdfs-native service does not expose hdfs-native's ClientBuilder::with_io_runtime (Kimahriman/hdfs-native#255); a TODO at the crate-owned runtime records that a runtime could be passed in once it does. Co-Authored-By: Claude Opus 5.5 <[email protected]>
github-merge-queue Bot
pushed a commit
to apache/iceberg-rust
that referenced
this pull request
Oct 22, 2026
* feat(storage): Add HDFS support via opendal services-hdfs-native Adds an opendal-hdfs-native cargo feature plus OpenDalStorageFactory::Hdfs and OpenDalStorage::Hdfs variants in iceberg-storage-opendal, using OpenDAL's services-hdfs-native (pure-Rust HDFS RPC, no JNI/libhdfs). The NameNode for a path resolves as: the hdfs.name-node property when set (comma-separated endpoints enable HA failover), otherwise the path authority. hadoop.-prefixed properties are forwarded to the HDFS client configuration, overriding values loaded from $HADOOP_CONF_DIR. Operators are cached per effective NameNode since each holds live RPC connections. Revives and updates PR #2441 (by @jordepic) against the current storage layer and opendal 0.58, where name_node became mandatory and the comma list is the HA mechanism. Closes #2440 Co-authored-by: Jordan Epstein <[email protected]> Co-Authored-By: Claude Opus 5 <[email protected]> * refactor: address review — Properties derive for HdfsConfig, drop HDFS CI special-casing - HdfsConfig is now pub(crate) and parsed via #[derive(Properties)] (key/prefix attributes) instead of hand-written TryFrom + TypedBuilder. - HDFS integration tests are no longer #[ignore]d and the dedicated CI step is gone; they run under the default nextest invocation since make docker-up already starts the fixture and the Tests job is Linux-only. Co-Authored-By: Claude Opus 5 <[email protected]> * fix: reject non-hierarchical hdfs URLs; key delete_stream batches by effective NameNode - Url::parse accepts non-hierarchical forms like `hdfs:x`; the byte-7 slice then panicked. Require the literal `hdfs://` prefix instead. - batch_key_for_path grouped by URL host only, so NameNodes differing by port shared one deleter; key by the effective NameNode (configured hdfs.name-node, else authority incl. port), matching the operator cache. Co-Authored-By: Claude Opus 5 <[email protected]> * refactor(storage): rename hdfs module to hdfs_native, drop unused HdfsConfig Per review: the opendal module, its functions and the storage/factory variants are now hdfs_native-prefixed (leaving room for a libhdfs-backed variant, see #1130), and the never-consumed HdfsConfig struct is removed from the core crate — config/hdfs.rs keeps only the property constants that iceberg-storage-opendal uses. Co-Authored-By: Claude Fable 5.1 <[email protected]> * ci: retrigger after setup-python manifest timeout on macos runner Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): address review — cache newtype, build outside lock, single NameNode rule, env-gated HDFS tests - HdfsNativeOperatorCache newtype keeps the cache representation out of the public API. - Operators are built outside the cache lock (the build reads Hadoop XML synchronously); a racing first caller's duplicate is dropped unopened. - hdfs_native_effective_name_node is the single source of the configured-else-authority rule for both create_operator and the delete_stream batch key; unresolvable paths key on themselves. - hdfs.name-node is trimmed and an empty value is treated as unset, so it no longer shadows the path-authority fallback. - HDFS integration tests self-skip unless ICEBERG_TEST_HDFS_ENDPOINT is set (as the HF tests do) and the compose services sit behind the hdfs profile; the Linux CI job opts in via env. Co-Authored-By: Claude Fable 5.1 <[email protected]> * chore(deps): bump rustls to 0.23.45 (RUSTSEC-2026-0285) Inherited from main (tracked in #3222); same bump as #3165. Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(storage): pin IPv6 authority handling in HDFS path parsing Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): resolve the HDFS NameNode in relativize_path too An authority-less path now errors in relativize_path exactly as in create_operator unless hdfs.name-node is set, so the two cannot disagree. Co-Authored-By: Claude Fable 5.1 <[email protected]> * docs(storage): state the cost of the synchronous HDFS operator build Co-Authored-By: Claude Fable 5.1 <[email protected]> * docs(storage): describe the HDFS operator cache lifecycle Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): normalize each entry of the hdfs.name-node list A space after a comma was passed through to opendal unchanged, which silently broke failover to that NameNode. Co-Authored-By: Claude Fable 5.1 <[email protected]> * feat(storage): accept PyIceberg's hdfs.host/hdfs.port for authority-less HDFS paths They map to Hadoop's fs.defaultFS and apply only when the path has no authority, as in PyIceberg; hdfs.name-node stays the explicit override. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): require a port in every hdfs.name-node entry An entry without a port can only fail at the first I/O as 'invalid socket address'; reject it at build time with the property and entry named. Bare host:port stays accepted, as in Hadoop and opendal. Co-Authored-By: Claude Fable 5.1 <[email protected]> * docs(storage): state the runtime binding of cached HDFS operators Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(storage): cover two-NameNode delete_stream batches and first-insert-wins caching Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(utils): name the HDFS port constant like its siblings Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): rebuild cached HDFS operators once their runtime is gone Each cache entry parks a sentinel task on the runtime that built the operator and keeps a Weak to a token the task owns. When that runtime shuts down the token dies, the entry reads as stale and the operator is rebuilt on the current runtime instead of hdfs-native panicking on the dead one. Sentinels are aborted when entries drop. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): resolve HDFS NameNodes like Hadoop does An authority with a port is used as is; hdfs.name-node serves logical nameservice authorities and authority-less paths, which otherwise fall back to fs.defaultFS. Portless strings from any source are logical names that only hdfs.name-node resolves, reported at resolution instead of at the first I/O. Co-Authored-By: Claude Fable 5.1 <[email protected]> * refactor(storage): keep the HDFS operator cache out of the public API OpenDalStorage::HdfsNative now wraps one opaque HdfsNativeStorage with crate-private fields, built only by the factories. The serialized form keeps its nesting; the io-timeout propagation test now covers the HDFS factory too. Co-Authored-By: Claude Fable 5.1 <[email protected]> * perf(storage): build HDFS operators on a blocking thread The build reads the Hadoop XML config synchronously; create_operator is now async and offloads it with spawn_blocking, outside the cache lock as before. The builder itself stays sync and runtime-free. Co-Authored-By: Claude Fable 5.1 <[email protected]> * ci: run the HDFS integration tests in their own job The default suite no longer pulls the Hadoop image or waits on the HDFS health checks; a dedicated required job starts only the two HDFS containers and runs the HDFS test binary. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): stop dropping HDFS properties silently PyIceberg's hdfs.user and hdfs.kerberos_ticket log a warning instead of vanishing (opendal's config has no user field), hdfs.port is validated whether or not a host is set and warns when it has no host to apply to, and a bare hadoop. key is rejected. Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(storage): derive the second NameNode spelling from the endpoint host Resolving the host to its IPv4 address gives any hostname endpoint a second batch key; an IP-literal endpoint skips the test explicitly instead of passing as a no-op. Co-Authored-By: Claude Fable 5.1 <[email protected]> * chore: record tracing as a dependency of iceberg-storage-opendal in Cargo.lock Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): report a missing tokio runtime instead of panicking in HDFS create_operator The runtime handle is resolved up front and passed into the cache, so the sentinel is no longer optional and the unreachable no-runtime branch is gone. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): normalize every HDFS NameNode spelling to hdfs://host:port One parser classifies configured entries, path authorities and fs.defaultFS: other schemes, logical names, port 0, userinfo, paths and unbracketed IPv6 are rejected, and bare and prefixed spellings share one cache key. Co-Authored-By: Claude Fable 5.1 <[email protected]> * feat(storage): declare HDFS nameservices with hdfs.name-node.<nameservice> The key expands to Hadoop's dfs.ha.namenodes and dfs.namenode.rpc-address keys in the forwarded options and the resolver reads them back, so nameservices declared through hadoop.* work the same way. Plain hdfs.name-node stays the documented single-cluster default; an undeclared name without it is a pointed error. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): require a declaration for every logical HDFS nameservice Plain hdfs.name-node serves authority-less paths only; an undeclared logical authority is an error naming the key to set instead of a guess that could reach the wrong cluster. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): reject userinfo and port 0 in HDFS path authorities Userinfo was silently dropped and port 0 was misread as a logical nameservice; both are DataInvalid now, and the userinfo message names the host only so a password never reaches a log. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): validate the NameNode composed from hdfs.host and hdfs.port Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(storage): use an authority-less path in the cross-runtime HDFS test A logical authority now requires a declaration, so the test must take the plain-key route. Co-Authored-By: Claude Fable 5.1 <[email protected]> * test(storage): tighten the HDFS tests after review Drop three redundant unit tests, assert the warnings and the password redaction directly, make the serde round trip populate the cache first, check which error each negative case produces, cover the plain key and hdfs.host serving authority-less paths against the fixture, and name the declared-nameservice test for what it does. Co-Authored-By: Claude Fable 5.1 <[email protected]> * fix(storage): mask userinfo in HDFS errors that echo a path or NameNode A path that failed to parse, or had another scheme, was echoed whole, so `hdfs://user:pw@nn:bad/x` put the password in the error. The NameNode config errors (`hdfs.name-node`, `hdfs.name-node.<nameservice>`, `hdfs.host`/`hdfs.port`, `fs.defaultFS`) echoed their values the same way. Every `Invalid hdfs path` error now goes through one helper, and the config errors use the same masking: everything between the scheme and the last `@` becomes `***`, so a password holding a raw `/`, `?`, `#` or `://` is covered too. Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix(storage): recover a poisoned HDFS operator cache lock A panic elsewhere while the cache lock was held turned every later HDFS call into a permanent `Unexpected` error. Writes only ever replace whole entries, so the map stays consistent: both lock sites now recover the guard with `PoisonError::into_inner`, and `get`/`insert` are infallible. Co-Authored-By: Claude Opus 5.5 <[email protected]> * test(storage): flush the fake NameNode before sampling dials The runtime test sampled its dial counter before the listener thread had necessarily counted every dial from the first runtime. It now drops that runtime and waits for the listener to close a marker connection; accepts are served in order, so every earlier dial is counted first. A failed accept no longer stops the listener. The runtime-shutdown unit test also asserts that the rebuilt operator is a new instance, not the stale one with a fresh sentinel. Co-Authored-By: Claude Opus 5.5 <[email protected]> * docs(storage): state where portless HDFS authorities differ from Hadoop Hadoop resolves a portless authority from hdfs-site.xml, else dials port 8020. Here it must be declared with `hdfs.name-node.<nameservice>`, because opendal builds the client as `hdfs://nameservice` and never hands hdfs-native the authority. The `HDFS_NAME_NODE` and resolver docs no longer claim Hadoop parity, the error says to add a port or declare the nameservice, and the README notes the limitation in its own note. Co-Authored-By: Claude Opus 5.5 <[email protected]> * refactor(storage): rename the HDFS config module to hdfs_native Matches its siblings, which are named after their storage feature (azdls, gcs, hf, oss, s3). The module is private and re-exported, so the public iceberg::io::HDFS_* paths are unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]> * test(storage): move the HDFS tests out of lib.rs into hdfs_native The serde round-trip and relativize_path tests now live next to the code they cover, with the module's test_hdfs_native_ prefix. Co-Authored-By: Claude Opus 5.5 <[email protected]> * refactor(storage): read HDFS properties through #[derive(Properties)] hdfs.name-node, the hdfs.name-node.<nameservice> and hadoop. prefixes, hdfs.host and hdfs.port are declared in one struct parsed by the Properties macro instead of being looked up by hand. Host and port stay trimmed strings so a blank value still counts as unset, as in PyIceberg. Validation, warnings and error messages are unchanged. Co-Authored-By: Claude Opus 5.5 <[email protected]> * ci: run the HDFS tests in the default suite like the other fixtures The HDFS fixture joins the shared bridge network with published ports instead of using host networking behind a compose profile, so make docker-up starts it on every platform: the DataNode advertises 127.0.0.1 and the tests set dfs.client.use.datanode.hostname. The tests no longer skip without ICEBERG_TEST_HDFS_ENDPOINT, and the dedicated Tests (hdfs) job is gone. Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix(storage): build HDFS clients in a crate-owned runtime An hdfs-native client spawns its NameNode connections on the runtime current when it is built and panics once that runtime is dropped. The cache used to detect that with a sentinel task per entry, which needed Handle::try_current, and the build ran under spawn_blocking, which made create_operator async for every backend. Clients are now built inside a crate-owned runtime started on first use, so a cached operator works from any caller's runtime: the sentinel, Handle::try_current and spawn_blocking are gone, and create_operator is sync again as on main. The Hadoop XML read (~0.1 ms) runs inline. Co-Authored-By: Claude Opus 5.5 <[email protected]> * refactor(storage): type HDFS path authorities and validate declarations at parse hdfs_native_parse_path returns an HdfsNativeAuthority (NameNode, Nameservice, Default), so the resolver matches on it instead of re-parsing the authority to learn whether it has a port. Every HA nameservice declaration, from hdfs.name-node.<nameservice> or passed through as hadoop.dfs.ha.namenodes.*, and the fs.defaultFS that serves authority-less paths are now validated when the config is parsed, so a broken one fails the first call rather than only the paths that use it. Co-Authored-By: Claude Opus 5.5 <[email protected]> * refactor(storage): drop index slicing and combinator chains in the HDFS backend The nameservice lookup is a for loop with let-else, and the userinfo masking and the relative-path extraction use split_once/rsplit_once instead of slicing at computed indices. A test pins that doubled slashes after the authority are still trimmed. Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix(storage): attach the hdfs.port parse error as the source The parse error was interpolated into the message; it is now attached as the error's source, which iceberg prints after the message. Co-Authored-By: Claude Opus 5.5 <[email protected]> * fix(storage): share one HDFS client across host name spellings host_str keeps the host's case for hdfs:// URLs, so hdfs://NN1:8020 and hdfs://nn1:8020 cached two clients for one NameNode. Endpoint hosts are now lowercased by one helper used by the config normalizer and the path parser, as Hadoop's FileSystem cache key does; nameservice names stay case-sensitive like the Hadoop keys that declare them. The cache's doc states that it is unbounded by design. Co-Authored-By: Claude Opus 5.5 <[email protected]> * docs(storage): note the upstream gap behind the HDFS client runtime opendal's hdfs-native service does not expose hdfs-native's ClientBuilder::with_io_runtime (Kimahriman/hdfs-native#255); a TODO at the crate-owned runtime records that a runtime could be passed in once it does. Co-Authored-By: Claude Opus 5.5 <[email protected]> * test(storage): make the HDFS HA test fail over for real The declared-nameservice test listed the single-node fixture twice, which drove opendal's list path but never needed a failover. Its first NameNode is now a local port nothing listens on, so every call has to fail over through opendal's synthetic nameservice; an opendal change that kept only the first entry of the list would fail it. Co-Authored-By: Claude Opus 5.5 <[email protected]> * chore: retrigger CI Docker Hub rate-limited the previous run's image pulls, so the default test job never started its fixtures. Co-Authored-By: Claude Opus 5.5 <[email protected]> * chore: retrigger CI The previous run hit Docker Hub's unauthenticated pull rate limit again before any test ran. Co-Authored-By: Claude Opus 5.5 <[email protected]> * docs(storage): document how to enable and configure the HDFS backend The iceberg-storage-opendal README gains an HDFS section: enabling the opendal-hdfs-native feature, the factory, the hdfs.* and hadoop.* properties, the Hadoop config, user and Kerberos it reads, and the portless-authority limitation. The experimental-backends note now points to it. Co-Authored-By: Claude Opus 5.5 <[email protected]> * docs(storage): drop the HDFS README section and link follow-up issues Configuration docs for every backend are tracked in #3510; the README keeps the one-line note on the portless-authority limitation. The TODO on the crate-owned HDFS runtime now points at #3509. Co-Authored-By: Claude Opus 5.5 <[email protected]> --------- Co-authored-by: Jordan Epstein <[email protected]> Co-authored-by: Claude Opus 5 <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #254