feat(core)!: implement write returns metadata #5562

meteorgan · 2025-01-19T09:24:56Z

Which issue does this PR close?

Part of #5557

Rationale for this change

What changes are included in this PR?

implement write returns metadata for Operator and BlockingOperator
enable write_has_content_length for all services and implement the logic for them
implement write returns metadata logic for service S3, fs, monoiofs and webhdfs
other services return default Metadata after writing
skip return value for language bindings

Are there any user-facing changes?

Yes.

write, write_with in Operator returns Result<Metadata> instead of Return<()>
Writer.close() returns Return<Metadata> instead of Return<()>
write in BlockingOperator returns Return<Metadata> instead of Return<()>
FunctionWrite.call() used by BlockingOperator returns Return<Metadata> instead of Return<()>

meteorgan · 2025-01-19T15:22:19Z

emmm, another issue related to ceph rados: the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Xuanwo · 2025-01-20T03:40:47Z

the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Well, well, well...

Let's do our best to permit this since it's not harmful. Technically, we don't allow users to parse the ETag value; instead, they should store it and compare it with values from the same source. I'm guessing the issue lies in part of our code assuming that the ETag must contain "?

meteorgan · 2025-01-20T05:20:26Z

the Etag returned by CompleteMultipartUpload doesn't follow the standard.

Well, well, well...

Let's do our best to permit this since it's not harmful. Technically, we don't allow users to parse the ETag value; instead, they should store it and compare it with values from the same source. I'm guessing the issue lies in part of our code assuming that the ETag must contain "?

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

Xuanwo · 2025-01-20T05:43:24Z

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

That's really surprising. Let's disable the Ceph RADOS test and create an issue to track it. I believe such a bug isn't worth our workaround.

meteorgan · 2025-01-20T13:05:10Z

That's really surprising. Let's disable the Ceph RADOS test and create an issue to track it. I believe such a bug isn't worth our workaround.

I've done some tests. it's very interesting that stat_with().if_match(etag) works fine regardless of whether the ETag includes the prefix and suffix " or not.

meteorgan · 2025-01-20T13:08:17Z

I've done some tests. it's very interesting that stat_with().if_match(etag) works fine regardless of whether the ETag includes the prefix and suffix " or not.

The results are the same across Ceph RADOS, Minio and S3

meteorgan · 2025-01-20T14:03:28Z

That's really surprising. Let's disable the Ceph RADOS test and create an issue to track it. I believe such a bug isn't worth our workaround.

I still created a workaround by adding prefix and suffix " for Etag in the test if they are not included. I think this approach is simpler, what do you think ?

Xuanwo · 2025-01-20T14:49:02Z

core/src/raw/adapters/kv/backend.rs

-        self.kv.set(&self.path, buf).await
+        self.kv.set(&self.path, buf).await?;
+
+        Ok(Metadata::default())


Maybe we can return at least the content length of file?

Originally, I planned to leave this for later PRs, as I was only prepared to implement the logic for S3. However, since content_length is not an Option, I agree with you - it's better to return content_length as the very least; otherwise users might be confused by the zero length.

But there is still an issue - what about append ? Can we have a simple method to retrieve the content_length without performing a stat or parsing the result of append ?

Xuanwo · 2025-01-20T14:49:12Z

core/src/raw/adapters/kv/backend.rs

        let buf = self.buffer.clone().collect();
        self.kv.blocking_set(&self.path, buf)?;
-        Ok(())
+        Ok(Metadata::default())


Xuanwo · 2025-01-20T14:49:25Z

core/src/raw/adapters/typed_kv/backend.rs

@@ -301,7 +301,8 @@ impl<S: Adapter> oio::Write for KvWriter<S> {
            }
        };
        self.kv.set(&self.path, value).await?;
-        Ok(())
+
+        Ok(Metadata::default())


We can return back the metadata in value.

Good point, thank you.

Xuanwo · 2025-01-20T14:49:32Z

core/src/raw/adapters/typed_kv/backend.rs

@@ -330,7 +331,7 @@ impl<S: Adapter> oio::BlockingWrite for KvWriter<S> {
        };

        kv.blocking_set(&self.path, value)?;
-        Ok(())
+        Ok(Metadata::default())


Xuanwo · 2025-01-20T14:51:56Z

core/src/raw/oio/write/append_write.rs

-    async fn close(&mut self) -> Result<()> {
-        Ok(())
+    async fn close(&mut self) -> Result<Metadata> {
+        Ok(Metadata::default())


Perhaps we should return valid data here. The metadata could be returned and replaced by self.inner.append().await, then stored within AppendWriter until the user calls close.

Xuanwo · 2025-01-20T14:54:37Z

core/src/services/aliyun_drive/writer.rs

        let (Some(upload_id), Some(file_id)) = (self.upload_id.as_ref(), self.file_id.as_ref())
        else {
-            return Ok(());
+            return Ok(Metadata::default());


I believe we need to set a content length for each service, at the very least. content_length is specifically handled in OpenDAL, where users will receive 0 if it doesn't set.

Xuanwo · 2025-01-20T14:56:52Z

core/src/services/s3/writer.rs

                if !maybe_error.code.is_empty() {
                    return Err(from_s3_error(maybe_error, parts));
                }

-                Ok(())
+                let ret: CompleteMultipartUploadResult =


Maybe we can embed the error struct in CompleteMultipartUploadResult so that we don't need to parse twice.

Xuanwo · 2025-01-20T14:57:31Z

core/tests/behavior/async_write.rs

+            last_modified_time
+        );
+    }
+    if let Some(etag) = meta.etag() {


I prefer to comment this test until rados fix it.

Xuanwo · 2025-01-20T14:58:23Z

core/tests/behavior/async_write.rs

+        assert_eq!(stat_meta.content_length(), meta.content_length());
+    }
+    if let Some(last_modified_time) = meta.last_modified() {
+        assert_eq!(


Maybe it's more readable to use the following?

assert_eq!(meta.last_modified(), stat_meta.last_modified())

Xuanwo · 2025-01-20T15:05:19Z

Perhaps we should introduce write_has_content_type flags to indicate which metadata is available in the meta returned by the write operation. Checking the meta.content_type() may pass the test but is meaningless to users; it could also potentially conceal incorrect implementations that developers can simplely return Metadata::default() to pass all tests. And it makes it more difficult for us to determine the implementation status of various services.

meteorgan · 2025-01-21T12:34:26Z

Perhaps we should introduce write_has_content_type flags to indicate which metadata is available in the meta returned by the write operation.

Would write_returns_content_type be a better name?

Checking the meta.content_type() may pass the test but is meaningless to users; it could also potentially conceal incorrect implementations that developers can simplely return Metadata::default() to pass all tests.

if a service returns Metadata::default(), it indicates that it implements nothing. There's a chance that a developer intended to return some contents but forgot set the fields. Unfortunately, the tests can't catch this.

And it makes it more difficult for us to determine the implementation status of various services.

Make sense ! Making it explicit is better.

meteorgan · 2025-01-21T12:46:06Z

No. The issue is that I compare the Etag from stat with the one from write in the tests to ensure we get the correct Etag if it's returned by write. However, the value returned from stat contain " while the value returned from write does not. they aren't consistent with each other, even within the same service !

Wow, they've already had a PR for this issue: ceph/ceph#60477

meteorgan · 2025-01-24T11:33:32Z

Gently push. Hi, @Xuanwo I've provided some replies, do you have any suggestions ?

Xuanwo · 2025-01-24T12:22:22Z

Would write_returns_content_type be a better name?

I prefer to align with exists stat_has_xxx and list_has_xxx.

Wow, they've already had a PR for this issue: ceph/ceph#60477

That's nice, maybe we can disable ceph test for now...

…e_returns_metadata logic for fs

meteorgan added services/s3 core breaking-changes labels Jan 19, 2025

github-actions bot added the releases-note/feat label Jan 19, 2025

meteorgan force-pushed the write_returns_metadata_for_s3 branch from 8591861 to 5c98d37 Compare January 19, 2025 10:44

meteorgan force-pushed the write_returns_metadata_for_s3 branch from 81aeb13 to ddc3de5 Compare January 20, 2025 13:47

meteorgan marked this pull request as ready for review January 20, 2025 14:03

meteorgan requested review from PsiACE, messense, Zheaoli, suyanhanx, tisonkun, silver-ymz and Xuanwo as code owners January 20, 2025 14:03

Xuanwo reviewed Jan 20, 2025

View reviewed changes

meteorgan added 3 commits January 26, 2025 20:53

feat(core): implement write returns metadata for s3

17a9e99

fix cargo clippy

b94a1ed

fix bindings: cpp, dotnet, haskell, ocaml, ruby

e3b0a25

meteorgan added 5 commits January 26, 2025 20:53

fix bindings: java, nodejs, python

e009fa1

fix binding cpp

0fddf1a

fix test_retry_write_fail_on_close

164cece

fix tests for Ceph RADOS and fix complete.rs

f2a1331

set write_has_content_length as true by default && implement the writ…

a1e0b0a

…e_returns_metadata logic for fs

meteorgan force-pushed the write_returns_metadata_for_s3 branch from ddc3de5 to a1e0b0a Compare January 26, 2025 12:53

implement logic for monoiofs

ff13a8f

meteorgan changed the title ~~feat(core)!: implement write returns metadata for s3~~ feat(core)!: implement write returns metadata Jan 26, 2025

meteorgan mentioned this pull request Jan 26, 2025

Tracking Issue: Ceph RADOS returns invalid Etag when calling CompleteMultipartUpload #5575

Open

implement logic for webhdfs

9482e08

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(core)!: implement write returns metadata #5562

feat(core)!: implement write returns metadata #5562

meteorgan commented Jan 19, 2025 •

edited

Loading

meteorgan commented Jan 19, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Xuanwo Jan 20, 2025

meteorgan Jan 21, 2025

meteorgan Jan 21, 2025

Xuanwo Jan 20, 2025

Xuanwo Jan 20, 2025

meteorgan Jan 21, 2025

Xuanwo Jan 20, 2025

Xuanwo Jan 20, 2025

Xuanwo Jan 20, 2025

Xuanwo Jan 20, 2025

Xuanwo Jan 20, 2025

Xuanwo Jan 20, 2025

Xuanwo commented Jan 20, 2025 •

edited

Loading

meteorgan commented Jan 21, 2025 •

edited

Loading

meteorgan commented Jan 21, 2025

meteorgan commented Jan 24, 2025

Xuanwo commented Jan 24, 2025

feat(core)!: implement write returns metadata #5562

Are you sure you want to change the base?

feat(core)!: implement write returns metadata #5562

Conversation

meteorgan commented Jan 19, 2025 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

meteorgan commented Jan 19, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Xuanwo commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

meteorgan commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xuanwo commented Jan 20, 2025 • edited Loading

meteorgan commented Jan 21, 2025 • edited Loading

meteorgan commented Jan 21, 2025

meteorgan commented Jan 24, 2025

Xuanwo commented Jan 24, 2025

meteorgan commented Jan 19, 2025 •

edited

Loading

Xuanwo commented Jan 20, 2025 •

edited

Loading

meteorgan commented Jan 21, 2025 •

edited

Loading