Repository navigation
Conversation
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 apache#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 apache#2440 Co-authored-by: Jordan Epstein <jordepic@users.noreply.github.com> Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
blackmwk
left a comment
There was a problem hiding this comment.
I found two correctness issues that need to be addressed before merge: malformed hdfs: URLs can panic, and bulk deletion can conflate NameNodes running on different ports. Details and suggested regression coverage are inline. Please also resolve the two existing review threads.
This review was drafted by an AI-assisted tool and confirmed by an Apache Iceberg Rust maintainer. After you've addressed the points above and pushed an update, an Apache Iceberg Rust maintainer — a real person — will take the next look at the PR. The findings cite the project's review criteria; if you think one of them is mis-applied, please reply on the PR and a maintainer will weigh in.
More on how Apache Iceberg Rust handles maintainer review: CONTRIBUTING.md.
…S 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 <noreply@anthropic.com>
…eat-storage-hdfs-native
…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 <noreply@anthropic.com>
# Conflicts: # Cargo.lock
…sConfig 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 apache#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 <noreply@anthropic.com>
|
@blackmwk |
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
comphead
left a comment
There was a problem hiding this comment.
Reviewed the backend against the locked opendal-service-hdfs-native 0.58.1 and hdfs-native 0.14.5 sources, and against the existing hf/oss/azdls backends in this crate.
The overall shape fits the crate well: *_config_parse / *_batch_key / (Operator, &str) mirror hf, and the dedicated batch_key arm is genuinely needed, since host-only keying would merge NameNodes that differ only by port. The operator cache is justified too, given each client re-reads the Hadoop XML and opens RPC connections.
Leaving 5 inline comments: 2 blockers (test gating versus what the description claims, and blocking I/O under the cache write lock) and 3 majors (the cache leaking into the public API, duplicated NameNode-precedence logic, and empty-name-node handling).
I also have a handful of minor simplification notes and two questions (operator caching pins the Client to the tokio runtime that first built it, since hdfs-native captures Handle::try_current() eagerly; and the hadoop. prefix scope). Happy to add those if useful, but they seemed like noise next to the above.
…gle 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 <noreply@anthropic.com>
Inherited from main (tracked in apache#3222); same bump as apache#3165. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Thanks @comphead for the thorough review — all five items are addressed in d17ae38 and answered inline. The "Bumped to 0.23.45 in b8cc860, the same fix #3165 applied" if we keep it without it CI will stay red, tell me if we want to revert this under this PR |
# Conflicts: # Cargo.lock
|
@comphead @blackmwk any other comments? I'm testing this with datafusion-comet, so far no issues in this code. |
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…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 <noreply@anthropic.com>
|
Thanks @laskoviymishka. All 10 threads are addressed in 22db8e6..9c62f4c (merge of main plus 9 commits), each with an inline reply. Behavior changes: the operator cache rebuilds entries whose tokio runtime is gone instead of hdfs-native panicking (regression test included); NameNode resolution follows Hadoop, so a |
…argo.lock Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
laskoviymishka
left a comment
There was a problem hiding this comment.
Thanks for the quick turnaround — a lot of the round-1 list landed cleanly. The hdfs: URL panic is gone (nice coverage on hdfs:x/hdfs:/x/hdfs:), batch deletes key on the effective NameNode so the port conflation is fixed, relativize_path and create_operator now share hdfs_native_effective_name_node, the HA list is trimmed per entry, the variant is HdfsNative, and the cache no longer leaks into the public API. On my IPv6 flag from last round — you were right and I was wrong: Url::host_str() does keep the brackets, and the hdfs://[::1]:8020 tests pin it.
Two things still open, one of them new. My executor-blocking point got addressed by moving the XML read into spawn_blocking — but that trades it for a panic when there's no tokio runtime, and it leaves the sentinel: None / "built outside any runtime" branch dead in production. That one I'd like resolved before merge. The portless-authority / logical-nameservice resolution is the carried-over half of my hdfs.name-node-vs-PyIceberg note, now sharper: with hdfs.name-node set every portless authority routes to the one configured cluster (risky for delete_*), and a nameservice living only in $HADOOP_CONF_DIR gets rejected — both hinge on how opendal 0.58.1 treats a bare logical name_node, which none of us could confirm. Details inline.
Everything else is a tightening nit (the hdfs_native_has_port validation + cache-key normalization). Sort the spawn_blocking panic and settle the portless-authority question and I think this is basically there.
…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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…vice> 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 <noreply@anthropic.com>
|
Thanks @laskoviymishka. The three threads from this round are addressed in 9c62f4c..ce78ee3, each with a reply inline: a missing tokio runtime is now a |
laskoviymishka
left a comment
There was a problem hiding this comment.
Thanks for the turnaround. Everything from round 1 is in now: IPv6 round-trips, shared hdfs_native_effective_name_node, HA list trimming, XML reads on spawn_blocking, and the @blackmwk / @comphead blockers are resolved.
One correctness issue left before I’d approve: with plain hdfs.name-node set, an unknown or mistyped authority falls back to that cluster. In a multi-cluster setup that can send writes/deletes to the wrong NameNode. hdfs_native_parse_path also allows userinfo and port 0, and hdfs://nn:0/... hits the same fallback. I think unknown authorities should error, not guess.
Non-blocking follow-ups:
- nameservices /
fs.defaultFSfrom$HADOOP_CONF_DIRXML are not visible to the resolver today. It already fails closed, so a doc note onHDFS_NAME_NODEis enough for now. - validate
hdfs.host/hdfs.portduring parsing.
Fix the fallback and the parse_path gap and I’m happy to approve.
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
…port Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
A logical authority now requires a declaration, so the test must take the plain-key route. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
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 <noreply@anthropic.com>
|
Thanks @laskoviymishka. Both blocking items and the non-blocking ones from this round are in ce78ee3..b962408: logical nameservices must be declared (plain |
laskoviymishka
left a comment
There was a problem hiding this comment.
This is in good shape, most of items from iteration landed. IPv6 round-trips (url.host_str keeping the brackets, with the [::1]:8020 test), relativize_path/create_operator/batch_key all go through hdfs_native_effective_name_node so they agree on authority-less paths, the HA list is trimmed per entry, and the XML read is in spawn_blocking. The shared set @blackmwk and @comphead had open — the hdfs: panic, port in the batch keys, the HdfsNative rename, the public-API cache leak — is in too. Thanks for working through all of it.
Good to land from me. I've left a few followup nits inline, none of them blocking:
- The portless-authority path:
hdfs://nameservice1/...(the common HA shape, where the nameservice is defined only in hdfs-site.xml underHADOOP_CONF_DIR) errors in the resolver instead of being handed to hdfs-native, which already reads the conf dir and resolves it. Worth passing undeclared portless authorities straight through so those clusters can read their own tables — or, if the strict behavior is deliberate for now, dropping the "as in Hadoop" doc line, since it diverges there. - The parse-error strings echo the full path (so a
user:pw@…path leaks the password) despite the comment promising otherwise. - A poisoned cache lock turns into permanent HDFS failure — recover the guard instead. Separate nuance from the lock thread I confirmed was fine last round, not a reversal.
- The runtime-rebuild test can go green without actually dialing from the second runtime.
I'll keep the PR soaking for at least a day before I merge, so there's room to fold any of these in now if you'd rather not leave them as follow-ons.
|
Ah, there is a blocking change_request from @blackmwk , so i'll wait for him to stamp before merging at least. |
Brings in origin/main up to 8c93fbb and moves the iceberg-rust pin to the head of apache/iceberg-rust#3111 (22db8e6f, HDFS via opendal hdfs-native). The branch's own Iceberg task-input and planning metrics commits are dropped in favour of main's equivalents (apache#5880, apache#6085, apache#5265). The pin also carries iceberg-rust main changes, so two Comet adaptations: FileWrite::close now returns FileMetadata (LocationScopedFileWrite) and UnboundPartitionField is built through its builder (planner tests). Also: CometIcebergHdfsSuite is registered in the CI workflows, the HDFS case in CometIcebergWriteDetectionSuite no longer treats hdfs as an unsupported scheme, and the Iceberg HDFS docs are corrected. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Brings in origin/main up to f7eb8aa and moves the iceberg-rust pin to the current head of apache/iceberg-rust#3111 (b9624086), which contains main's own pin (af1da4c5) plus the HDFS commits. That head changes how HDFS NameNodes resolve: a portless path authority is a logical nameservice resolved only through its declaration, hdfs.name-node.<nameservice> (or Hadoop's dfs.ha.namenodes keys passed as hadoop.*); the global hdfs.name-node now serves authority-less paths only, and there is no default port. Comet's HA translation emitted the global key, so the HA MiniDFS test failed with "logical nameservice ... is not declared". Comet now: - declares HA nameservices as hdfs.name-node.<nameservice>, derived from the session Hadoop configuration overlaid with the table FileIO's dfs.* keys; - declares a portless plain host on port 8020, as the JVM client dials it, and no longer invents 8020 for a portless rpc-address, which the JVM rejects; - falls back at planning, instead of failing on an executor, for an undeclared nameservice, a NameNode entry that is not host:port in any hdfs.name-node* key, DNS-resolved NameNodes (resolve-needed), malformed hdfs.host/hdfs.port or a bare hadoop. key, and a declaration of opendal's synthetic nameservice name. The HA test now also checks the declaration before executing and keeps reading and writing after the first NameNode is shut down. The pre-epoch partition tripwire follows iceberg-rust apache#3323, which the pin carries; Comet's own values are unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
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 <noreply@anthropic.com>
Which issue does this PR close?
This revives #1131's successor #2441 by @jordepic (closed by the stale bot after a first review round), rebased onto the current
Storage-trait layout and updated for opendal 0.58, whereservices-hdfs-nativechanged behavior in ways that required design changes (details below).What changes are included in this PR?
Adds an
opendal-hdfs-nativecargo feature toiceberg-storage-opendal, withOpenDalStorageFactory::HdfsNative/OpenDalStorage::HdfsNativevariants andhdfs://routing inOpenDalResolvingStorage. The backend uses OpenDAL'sservices-hdfs-native(pure-Rust HDFS RPC viahdfs-native— no JNI/libhdfs). The feature is experimental and not part ofopendal-all.NameNode resolution (differs from #2441, forced by opendal's hdfs-native service, where
name_nodeis mandatory and, since 0.56 (apache/opendal#7248), its comma-split list is the HA mechanism):hdfs://host:port/path) is used as is; a configuredhdfs.name-nodenever overrides it. Userinfo and port 0 in an authority are rejected withDataInvalid(every error the HDFS backend raises for a path or NameNode value masks everything between the scheme and the last@, e.g.hdfs://***@nn:8020/x).hdfs://ns1/path) must be a declared nameservice:hdfs.name-node.<nameservice>takes a single endpoint or a comma-separated list for HA failover and is sugar for Hadoop'sdfs.ha.namenodes.<nameservice>/dfs.namenode.rpc-address.<nameservice>.<id>, which are honored as well when passed throughhadoop.*. Unlike Hadoop, an undeclared one is an error naming the key to set, rather than being resolved from$HADOOP_CONF_DIRor dialed on port 8020: opendal always builds the client ashdfs://nameservicefrom an explicit list, so hdfs-native never sees the path's authority (noted in the README; lifting it needs opendal to accept a logical nameservice asname_node). Every NameNode spelling, configured or from a path, is normalized tohdfs://host:portso all sources share cache keys; other schemes, userinfo, paths, port 0 and unbracketed IPv6 are rejected, and hdfs-native has no default port, so a portless NameNode can only be a logical name.hdfs:///path) resolves through plainhdfs.name-node(newHDFS_NAME_NODE/HDFS_HADOOP_CONF_PREFIXconstants iniceberg::io), elsefs.defaultFS: set from PyIceberg'shdfs.host/hdfs.port(newHDFS_HOST/HDFS_PORTconstants, port defaults to 8020, validated at config time; as in PyIceberg they apply only without a path authority) or fromhadoop.fs.defaultFS. Anything unresolvable is rejected at resolution time with an error naming the property to set.hadoop.-prefixed properties pass through to the HDFS client config with the prefix stripped, overriding$HADOOP_CONF_DIRvalues (mirroring thehadoop.catalog-property convention of the Java integrations);hdfs-nativestill loadscore-site.xml/hdfs-site.xmlfrom$HADOOP_CONF_DIR/$HADOOP_HOMEfor everything else, and Kerberos works vialibgssapi_krb5(runtime dlopen). PyIceberg'shdfs.user/hdfs.kerberos_tickethave no equivalent in opendal's config and log a warning.Operators are cached per effective NameNode inside an opaque
HdfsNativeStorage(built only by the factories) and built on a blocking thread, since the build reads the Hadoop XML synchronously. Each cached client is bound to the tokio runtime that built it; the cache notices when that runtime is gone and rebuilds the operator, because hdfs-native panics when spawning onto a dead runtime. A poisoned cache lock is recovered rather than failing every later call. Without a tokio runtime,create_operatorreturnsFeatureUnsupportedinstead of panicking (every backend's I/O needs one for opendal's timeout layer).Relative paths are returned opendal-style without a leading
/—opendal::Deleter::delete(used bydelete_stream) rejects leading slashes, which an integration test caught.Test infrastructure: single-node HDFS docker fixture (
apache/hadoop:3.5.0, host networking — required becausehdfs-nativedials DataNodes by their registered IP, unroutable on a bridge). The DataNode healthcheck gates on NameNode registration so--waitmeans writable. The fixture is opt-in (profiles: [hdfs], started withCOMPOSE_PROFILES=hdfs make docker-up) and the tests self-skip unlessICEBERG_TEST_HDFS_ENDPOINTis set, like the HF tests. A dedicatedTests (hdfs)CI job starts only the two HDFS containers and sets the endpoint, so the default suite no longer pulls the Hadoop image.Are these changes tested?
hdfs.host/hdfs.portintofs.defaultFS, rejected and ignored keys), path parsing (authority/port/IPv6/authority-less/non-hierarchical), NameNode precedence and nameservice declarations, spelling normalization, operator caching (first-insert-wins for racing builds, rebuild after runtime shutdown, sentinel cleanup, poisoned-lock recovery), userinfo masking in errors, serde round-trip, batch keys, relativize, and scheme resolution.hdfs.name-nodeandhdfs.host/hdfs.port, resolving storage overhdfs://, and the HA flow (logical authority in the path +hdfs.name-nodeproperty), plus a fixture-free test that reuses aFileIOfrom a second runtime after the first is dropped (it panics on the old cache). The docker-backed suite runs in theTests (hdfs)CI job and self-skips elsewhere unlessICEBERG_TEST_HDFS_ENDPOINTis set.make check(incl. cargo-deny licenses), full-workspace unit/doc tests, MSRV, the public API check, and the s3/gcs/hms/resolving integration suites pass locally;public-api.txtregenerated for both crates.AI Disclosure
Developed with AI assistance (Claude Code): drafting code/tests/fixtures starting from #2441, and cross-checking the design against the locked opendal 0.58.1 / hdfs-native 0.14.6 sources. I reviewed the implementation and ran all verification locally. Areas worth reviewer attention: the NameNode-resolution semantics above (opendal's synthetic-nameservice behavior constrains what
hdfs://<nameservice>paths can do without the property), and the Windows--all-featuresbuild ofhdfs-native, which I could only verify via CI.🤖 Generated with Claude Code