You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by xu...@apache.org on 2026/08/23 08:28:12 UTC
(opendal) branch main updated: fix(services/goosefs): skip CreateDirectory when rename parent already exists (#8129)
This is an automated email from the ASF dual-hosted git repository.
Xuanwo pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/opendal.git
The following commit(s) were added to refs/heads/main by this push:
new 5dd49f523 fix(services/goosefs): skip CreateDirectory when rename parent already exists (#8129)
5dd49f523 is described below
commit 5dd49f5231931b4556adda6eaefb92e41b9c4662
Author: ForwardXu <fo...@apache.org>
AuthorDate: Sun Aug 23 16:28:04 2026 +0800
fix(services/goosefs): skip CreateDirectory when rename parent already exists (#8129)
CACHE_THROUGH make_directory on an existing parent is a CosN round-trip, not a metadata no-op. Create the destination parent only if Master reports it missing after rename.
---
core/services/goosefs/src/core.rs | 65 +++++++++++++++++-----
core/services/goosefs/src/docs.md | 3 +
.../services/goosefs/tests/safe_conditional_put.rs | 37 ++++++++++++
3 files changed, 91 insertions(+), 14 deletions(-)
diff --git a/core/services/goosefs/src/core.rs b/core/services/goosefs/src/core.rs
index 3891e62f9..75bdb357c 100644
--- a/core/services/goosefs/src/core.rs
+++ b/core/services/goosefs/src/core.rs
@@ -260,7 +260,12 @@ impl GoosefsCore {
/// Mirror HDFS / Alluxio semantics so GooseFS passes OpenDAL's behavior
/// contract for rename:
///
- /// 1. Destination parent directory is created on demand.
+ /// 1. Destination parent directory is created on demand — but only when
+ /// Master reports it missing. Under `CACHE_THROUGH`,
+ /// `create_directory` on an already-persisted parent is a CosN
+ /// round-trip (tens of milliseconds), not a metadata no-op. The
+ /// write-via-temp path already created that parent via
+ /// `CreateFile(recursive)`, so the publish `rename` skips mkdir.
/// 2. If the destination exists as a file and `if_not_exists` is false,
/// overwrite by deleting it first. Never delete when `if_not_exists`
/// is true (that would destroy Master no-replace and re-introduce
@@ -304,22 +309,13 @@ impl GoosefsCore {
// delete first so overwrite can proceed.
master.delete(&dst, false).await.map_err(parse_error)?;
}
- Err(goosefs_sdk::error::Error::NotFound { .. }) => {
- // Ensure the destination's parent directory exists.
- // `create_directory` with recursive=true is idempotent,
- // so calling it for a parent that already exists is a
- // cheap metadata no-op on the master.
- if let Some(parent) = parent_of(&dst) {
- master
- .create_directory(parent, true)
- .await
- .map_err(parse_error)?;
- }
- }
+ // Parent may already exist (write-via-temp always does). Create it
+ // only if the subsequent rename reports it missing.
+ Err(goosefs_sdk::error::Error::NotFound { .. }) => {}
Err(e) => return Err(parse_error(e)),
}
- match master.rename(&src, &dst).await {
+ match rename_creating_parent_if_missing(&master, &src, &dst).await {
Ok(()) => {
// Same inode id moved (Master RenameEntry.setId(srcInode.getId()))
// — not a fresh inode. Drop any cached FileInfo so a reader
@@ -541,6 +537,40 @@ impl GoosefsCore {
}
}
+/// Run `rename(src, dst)`, creating `dst`'s parent only if Master reports
+/// the destination path as missing.
+///
+/// The write-via-temp publish path never takes the mkdir branch: tmp lives
+/// in the same directory as `dst`, and `CreateFile(recursive)` already
+/// created that parent. Eager `create_directory` on an existing parent is
+/// the 45–55ms `CACHE_THROUGH` no-op this helper exists to skip.
+///
+/// `rename` NotFound is also what Master returns for a missing source, so
+/// we `get_status(src)` before mkdir and only create the parent when the
+/// source still exists.
+async fn rename_creating_parent_if_missing(
+ master: &MasterClient,
+ src: &str,
+ dst: &str,
+) -> goosefs_sdk::error::Result<()> {
+ match master.rename(src, dst).await {
+ Ok(()) => Ok(()),
+ Err(e) if matches!(e, goosefs_sdk::error::Error::NotFound { .. }) => {
+ let Some(parent) = parent_of(dst) else {
+ return Err(e);
+ };
+ match master.get_status(src).await {
+ Ok(_) => {}
+ Err(goosefs_sdk::error::Error::NotFound { .. }) => return Err(e),
+ Err(status_err) => return Err(status_err),
+ }
+ master.create_directory(parent, true).await?;
+ master.rename(src, dst).await
+ }
+ Err(e) => Err(e),
+ }
+}
+
/// Return the parent directory of an absolute GooseFS path, or `None`
/// when the path has no meaningful parent to create (i.e. it already
/// points at the filesystem root).
@@ -588,6 +618,13 @@ mod tests {
assert_eq!(parent_of("/a/b/c"), Some("/a/b"));
assert_eq!(parent_of("/a/b/c/"), Some("/a/b"));
assert_eq!(parent_of("/a/b/c/d"), Some("/a/b/c"));
+ // Write-via-temp publish: dst is a sibling of `.opendal.tmp.*` under
+ // an already-created parent, so retry-mkdir is gated on this being Some
+ // but is not taken on the happy path.
+ assert_eq!(
+ parent_of("/trace_test/03/data/file.lance"),
+ Some("/trace_test/03/data")
+ );
}
#[test]
diff --git a/core/services/goosefs/src/docs.md b/core/services/goosefs/src/docs.md
index 1f78290e7..1acdc9cec 100644
--- a/core/services/goosefs/src/docs.md
+++ b/core/services/goosefs/src/docs.md
@@ -29,6 +29,9 @@ Features:
- **All WriteTypes**: Supports MUST_CACHE, CACHE_THROUGH, THROUGH, and ASYNC_THROUGH.
- **Conditional Create**: `write_with_if_not_exists` publishes via Master no-replace
rename (`rename_with_if_not_exists`); destination is never deleted on the Create path.
+- **Rename parent directories**: `rename` creates a missing destination parent only
+ when Master reports it missing. Write-via-temp publish does not call
+ `CreateDirectory` for a parent that `CreateFile(recursive)` already created.
## Configuration
diff --git a/core/services/goosefs/tests/safe_conditional_put.rs b/core/services/goosefs/tests/safe_conditional_put.rs
index 23502aedc..9f08c923c 100644
--- a/core/services/goosefs/tests/safe_conditional_put.rs
+++ b/core/services/goosefs/tests/safe_conditional_put.rs
@@ -197,3 +197,40 @@ async fn concurrent_write_if_not_exists_exactly_one_wins() {
let _ = op.delete(&p).await;
}
}
+
+/// Nested write: CreateFile(recursive) on the temp sibling creates the parent,
+/// so publish rename must succeed without an eager CreateDirectory.
+#[tokio::test]
+async fn write_nested_path_creates_parents() {
+ let Some(op) = maybe_operator() else {
+ eprintln!("skip: OPENDAL_GOOSEFS_MASTER_ADDR unset");
+ return;
+ };
+
+ let path = format!("{}/nested/file", unique("nested-write"));
+ op.write(&path, "nested-body")
+ .await
+ .expect("write into missing parents");
+ let body = op.read(&path).await.expect("read").to_bytes();
+ assert_eq!(&body[..], b"nested-body");
+ let _ = op.delete(&path).await;
+}
+
+/// Public rename still creates a destination parent that does not exist yet.
+#[tokio::test]
+async fn rename_creates_missing_destination_parent() {
+ let Some(op) = maybe_operator() else {
+ eprintln!("skip: OPENDAL_GOOSEFS_MASTER_ADDR unset");
+ return;
+ };
+
+ let src = unique("mkdir-src");
+ op.write(&src, "payload").await.expect("write src");
+ let dst = format!("{}/nested/file", unique("mkdir-dst"));
+ op.rename(&src, &dst)
+ .await
+ .expect("rename should create missing parent");
+ let body = op.read(&dst).await.expect("read dst").to_bytes();
+ assert_eq!(&body[..], b"payload");
+ let _ = op.delete(&dst).await;
+}