feat(core)!: implement write returns metadata by meteorgan · Pull Request #5562 · apache/opendal

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, hdfs 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 ?

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

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

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

core/src/services/aliyun_drive/writer.rs

core/src/services/s3/writer.rs

core/tests/behavior/async_write.rs

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...

meteorgan · 2025-02-12T10:05:23Z

Hi, @Xuanwo Would you mind taking some time to review this PR at your convenience? since it involves a lot of files, it may become difficult to merge if it takes too long.

Xuanwo · 2025-02-12T11:40:41Z

Hi, @Xuanwo Would you mind taking some time to review this PR at your convenience? since it involves a lot of files, it may become difficult to merge if it takes too long.

Thanks a lot for your great work! I will review this PR after #5620 been addressed.

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

Xuanwo

Mostly LGTM! Only some small polishment changes to do.

core/src/layers/complete.rs

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

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

core/src/services/cos/writer.rs

core/src/services/hdfs/writer.rs

…e_returns_metadata logic for fs

…nderlying services don't return content_length

… no append

…a_for_s3 Signed-off-by: Xuanwo <[email protected]>

Xuanwo

Thank you @meteorgan for this great work!

Xuanwo

Thank you @meteorgan for this great work!

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

github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "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, Xuanwo, Zheaoli, messense, silver-ymz, suyanhanx and tisonkun as code owners January 20, 2025 14:03

Xuanwo reviewed Jan 20, 2025

View reviewed changes

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

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

Xuanwo reviewed Feb 14, 2025

View reviewed changes

core/src/raw/oio/write/multipart_write.rs Outdated Show resolved Hide resolved

meteorgan force-pushed the write_returns_metadata_for_s3 branch from 1773dd4 to ffcecb4 Compare February 16, 2025 12:30

Xuanwo reviewed Feb 17, 2025

View reviewed changes

meteorgan added 16 commits February 17, 2025 19:40

feat(core): implement write returns metadata for s3

e1ed7e5

fix cargo clippy

a99830f

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

7e4b504

fix bindings: java, nodejs, python

8248351

fix binding cpp

5f8b850

fix test_retry_write_fail_on_close

63adb06

fix tests for Ceph RADOS and fix complete.rs

e6d1ee7

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

72d979f

…e_returns_metadata logic for fs

implement logic for monoiofs

95caba1

implement logic for webhdfs

1ec96ae

implement the logic for hdfs

fdbf9b6

return content_length for aliyun_drive after writing

be484df

compute content_length in completeLayer when it's overwrite and the u…

6355e7f

…nderlying services don't return content_length

remove the code of computing content_length in services if there's is…

3444f04

… no append

fix hdfs

01165c2

add a check after writing && fix a latent bug when writing with retries

742c1e7

meteorgan force-pushed the write_returns_metadata_for_s3 branch from dae34cb to 742c1e7 Compare February 17, 2025 11:40

meteorgan and others added 2 commits February 17, 2025 22:45

add one chaos test for block_write and multipart_write

ee8e2e5

Merge remote-tracking branch 'origin/main' into write_returns_metadat…

8b8e63c

…a_for_s3 Signed-off-by: Xuanwo <[email protected]>

Xuanwo approved these changes Feb 18, 2025

View reviewed changes

Xuanwo merged commit 7455995 into apache:main Feb 18, 2025
244 checks passed

meteorgan deleted the write_returns_metadata_for_s3 branch February 18, 2025 09:31

meteorgan mentioned this pull request Feb 18, 2025

Tracking Issues of RFC-5556: Write Returns Metadata #5557

Open

9 tasks

meteorgan mentioned this pull request Mar 4, 2025

Implement Write Returns Metadata for all services #5693

Open

37 tasks

Comments

Conversation

meteorgan commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

meteorgan commented Jan 19, 2025

Uh oh!

Xuanwo commented Jan 20, 2025

Uh oh!

meteorgan commented Jan 20, 2025

Uh oh!

Xuanwo commented Jan 20, 2025

Uh oh!

meteorgan commented Jan 20, 2025

Uh oh!

meteorgan commented Jan 20, 2025

Uh oh!

meteorgan commented Jan 20, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xuanwo commented Jan 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meteorgan commented Jan 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meteorgan commented Jan 21, 2025

Uh oh!

meteorgan commented Jan 24, 2025

Uh oh!

Xuanwo commented Jan 24, 2025

Uh oh!

meteorgan commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Xuanwo commented Feb 12, 2025

Uh oh!

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

Xuanwo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

meteorgan commented Jan 19, 2025 •

edited

Loading

Xuanwo commented Jan 20, 2025 •

edited

Loading

meteorgan commented Jan 21, 2025 •

edited

Loading

meteorgan commented Feb 12, 2025 •

edited

Loading