services/cos: Implement Write Returns Metadata for cos by Ziy1-Tan · Pull Request #5710 · apache/opendal

Ziy1-Tan · 2025-03-08T08:53:20Z

Which issue does this PR close?

Part of #5557

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Ziy1-Tan · 2025-03-08T08:55:00Z

cc @meteorgan, would you mind taking a look at this PR? :)

meteorgan

LGTM. Thanks for this @Ziy1-Tan

Xuanwo · 2025-03-08T16:08:49Z

Hi, this PR's change seems wrong:

failures:

---- behavior::test_writer_return_metadata ----
test panicked: assertion `left == right` failed
  left: Some("\"a232d2c3a470308f9de0ce67a4574f7b-2\"")
 right: None

---- behavior::test_write_with_append_returns_metadata ----
test panicked: assertion `left == right` failed
  left: Some("null")
 right: None

---- behavior::test_blocking_write_with_append_returns_metadata ----
test panicked: assertion `left == right` failed
  left: None
 right: Some("null")


failures:
    behavior::test_writer_return_metadata
    behavior::test_write_with_append_returns_metadata
    behavior::test_blocking_write_with_append_returns_metadata

cc @meteorgan, PRs from forks can't run our integration tests, so we need a thorough double-check here. We don't have good solutions towards this now, it's sad.

Xuanwo · 2025-03-08T17:44:06Z

core/src/services/cos/writer.rs

            .cos_complete_multipart_upload(&self.path, upload_id, parts)
            .await?;

+        let meta = Self::parse_metadata(resp.headers())?;


It looks like cos returns ETag in the CompleteMultipartUploadResult instead of header.

https://cloud.tencent.com/document/product/436/7742

According to the documentation, Etag should be included in the common response headers. However, it's missing in the examples.

Hi, would youl like to share the docs? I failed to find it.

It's the docs you referenced ealier.

Xuanwo · 2025-03-08T17:47:56Z

core/src/services/cos/writer.rs


        let resp = self.core.send(req).await?;

+        let meta = Self::parse_metadata(resp.headers())?;


It's strange that COS doesn't claim to support append object. I couldn't find the API documentation and don't understand why we used to pass the tests.

Seems we are using the cos's non-public API: https://github.com/tencentyun/cos-go-sdk-v5/blob/e2423bebcf86fbf162ef04cf8c46b186ff05f593/object.go#L814-L827

So, COS didn't return an ETag for an append object (a special PutObject request), which differs from the behavior of a normal PutObject request.

In this document, the response includes an Etag, but there is no version_id.

It seems COS may return null as the version id when it's an appendable object.

Yep, cos doesn't support versioning for appendable object.

This reverts commit 5194625.

#5713) Revert "services/cos: Implement Write Returns Metadata for cos (#5710)" This reverts commit 5194625.

meteorgan · 2025-03-09T14:13:36Z

@Ziy1-Tan are you interested to fix these issues ? it's also better to test them locally.

Ziy1-Tan · 2025-03-10T14:41:35Z

@Ziy1-Tan are you interested to fix these issues ? it's also better to test them locally.

Sure, Let me take a look.

services/cos: Implement Write Returns Metadata for cos

803fce2

Ziy1-Tan requested a review from Xuanwo as a code owner March 8, 2025 08:53

meteorgan approved these changes Mar 8, 2025

View reviewed changes

Xuanwo merged commit 5194625 into apache:main Mar 8, 2025
93 checks passed

Xuanwo reviewed Mar 8, 2025

View reviewed changes

Xuanwo added a commit that referenced this pull request Mar 8, 2025

Revert "services/cos: Implement Write Returns Metadata for cos (#5710)"

adab4d4

This reverts commit 5194625.

Xuanwo added a commit that referenced this pull request Mar 8, 2025

chore: revert "services/cos: Implement Write Returns Metadata for cos" (

258982f

#5713) Revert "services/cos: Implement Write Returns Metadata for cos (#5710)" This reverts commit 5194625.

meteorgan mentioned this pull request Mar 15, 2025

services/cos: Implement Write Returns Metadata for cos #5777

Merged


		let resp = self.core.send(req).await?;

		let meta = Self::parse_metadata(resp.headers())?;

Comments

Conversation

Ziy1-Tan commented Mar 8, 2025

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!

Ziy1-Tan commented Mar 8, 2025

Uh oh!

meteorgan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Xuanwo Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xuanwo Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

meteorgan Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

Xuanwo Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

Xuanwo Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

Xuanwo Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Xuanwo Mar 9, 2025

Choose a reason for hiding this comment

Uh oh!

meteorgan commented Mar 9, 2025

Uh oh!

Ziy1-Tan commented Mar 10, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Xuanwo commented Mar 8, 2025 •

edited

Loading

meteorgan Mar 9, 2025 •

edited

Loading

meteorgan Mar 9, 2025 •

edited

Loading

meteorgan Mar 9, 2025 •

edited

Loading