feat(services/hdfs-native): support rename with if_not_exists - #8090
feat(services/hdfs-native): support rename with if_not_exists#8090PDGGK wants to merge 2 commits into
Conversation
Part of RFC-7818. The destination is stat'd before the rename and an existing file is deleted, so ConditionNotMatch has to be returned before that delete. The not-found arm pre-creates the destination with WriteOptions::default(), which does not overwrite, so a racing writer surfaces as AlreadyExists there; under if_not_exists that is mapped to ConditionNotMatch as well. Verified against the Hadoop 3.2.1 fixture images: 116 passed / 0 failed against 113 / 0 on an unmodified tree, the delta being exactly the three rename_with_if_not_exists behaviour tests.
| use opendal_core::raw::*; | ||
| use opendal_core::*; | ||
|
|
||
| fn map_hdfs_rename_error(err: HdfsError, if_not_exists: bool, to_path: &str) -> Error { |
|
It handles a race, and it is separable from the rest — dropped in the latest push, so this PR is now only the capability plus the check. For the record, what it did: in the not-found arm the destination is pre-created with It is not needed for the RFC behaviour: the three Happy to send it as its own PR if you want that error kind corrected; otherwise it can stay as it is. |
Which issue does this PR close?
Part of #7828 — RFC-7818. Said on #8089 that
hdfs_nativewould follow webdav; this is that.Rationale for this change
hdfs_nativetook_args: OpRenameand ignored it, soif_not_existswas rejected by the correctness-check layer. The siblinghdfsservice already implements this; the two now behave the same way.Two places needed the flag.
1. The destination is stat'd before the rename, and an existing file is deleted.
test_rename_with_if_not_exists_returns_condition_not_matchasserts the target still reads back with its original content, so returningConditionNotMatchhas to happen before the delete. The check is placed after theisdirarm, keeping the existingIsADirectoryanswer for a directory destination — same ordering ashdfs.2. The not-found arm pre-creates the destination, and that create can lose a race.
WriteOptions::default()isoverwrite: false(hdfs-native-0.14.3client.rs:53), so if another writer takes the destination between the stat and this call, the NameNode raisesFileAlreadyExistsException, which the crate maps toHdfsError::AlreadyExists(hdfs/proxy.rs:347) and this service maps toErrorKind::AlreadyExists. Underif_not_existsthe contract calls forConditionNotMatch, so a small mapper translates it — the same shape asmap_hdfs_rename_errorinservices/hdfs.The final
rename(..., overwrite = true)stays as it is: by that point the destination is a file this call just created, sooverwrite = falsewould reject our own placeholder.Verification
Against a real HDFS cluster, not by reading. The fixture in
fixtures/hdfs/docker-compose-hdfs-cluster.ymlusesnetwork_mode: host, which does not reach the host on Docker Desktop for macOS, so I ran the same two images on a bridge network withdfs.datanode.hostname=localhostanddfs.client.use.datanode.hostname=true. Same Hadoop 3.2.1 images, sameOPENDAL_HDFS_NATIVE_*variables as the CI action.Rename suite:
test_rename_overwriteis in that set, so the default path still replaces the destination.And the control that makes those numbers mean something — dropping the
ConditionNotMatchreturn while keeping the capability bit:Exactly one test. The check, not the capability bit, is doing the work.
Whole-suite counts, same cluster,
--test-threads 1, unmodified tree vs this branch:upstream/main113 passed; 0 failed116 passed; 0 failedThe three added tests are exactly the delta in the total, and nothing else moved.
Worth flagging for anyone reproducing this locally: run it single-threaded. With the default thread count a single-DataNode cluster on this machine fails a handful of read-stream tests — a different handful on each run, on the unmodified tree as well — so a parallel run is not a usable control.
cargo clippy -p opendal-service-hdfs-native --all-features --all-targets -- -D warningsandcargo fmtare clean.On not adding a unit test
Setting the capability is what enrolls the service in the three behaviour tests above, and those run against a real NameNode. A unit test over the error mapper would only restate the
if_not_exists &&guard that is visible in the diff. Happy to add one if you would rather have it.Separable
If you would rather land the minimum, the mapper in the not-found arm can be dropped and the stat-path check alone still passes all three behaviour tests — the mapper only changes which error a concurrent writer sees. I have kept it because
AlreadyExistsleaking out of anif_not_existscall is the one error kind that operation is defined not to produce.Are there any user-facing changes?
Yes —
op.rename_with(from, to).if_not_exists(true)now works onhdfs_native, returningErrorKind::ConditionNotMatchwhen the destination exists. Ordinaryrenameis unchanged.