You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "Ziy1-Tan (via GitHub)" <gi...@apache.org> on 2025/03/08 08:53:23 UTC
[PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Ziy1-Tan opened a new pull request, #5710:
URL: https://github.com/apache/opendal/pull/5710
# Which issue does this PR close?
<!--
We generally require a GitHub issue to be filed for all bug fixes and enhancements. You can link an issue to this PR using the GitHub syntax. For example `Closes #123` indicates that this PR will close issue #123.
-->
Part of https://github.com/apache/opendal/issues/5557
# Rationale for this change
<!--
Why are you proposing this change? If this is already explained clearly in the issue then this section is not needed.
Explaining clearly why changes are proposed helps reviewers understand your changes and offer better suggestions for fixes.
-->
# What changes are included in this PR?
<!--
There is no need to duplicate the description in the issue here but it is sometimes worth providing a summary of the individual changes in this PR.
-->
# Are there any user-facing changes?
<!--
If there are user-facing changes then we may require documentation to be updated before approving the PR.
-->
<!---
If there are any breaking changes to public APIs, please add the `breaking-changes` label.
-->
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986323934
##########
core/src/services/cos/writer.rs:
##########
@@ -139,10 +154,11 @@ impl oio::MultipartWrite for CosWriter {
.cos_complete_multipart_upload(&self.path, upload_id, parts)
.await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
Hi, would youl like to share the docs? I failed to find it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986306356
##########
core/src/services/cos/writer.rs:
##########
@@ -139,10 +154,11 @@ impl oio::MultipartWrite for CosWriter {
.cos_complete_multipart_upload(&self.path, upload_id, parts)
.await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
According to the documentation, `Etag` should be included in the common response headers. However, it's missing in the examples.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986317214
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
It seems COS may return `null` as the `version id` when it's an `appendable` object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986306356
##########
core/src/services/cos/writer.rs:
##########
@@ -139,10 +154,11 @@ impl oio::MultipartWrite for CosWriter {
.cos_complete_multipart_upload(&self.path, upload_id, parts)
.await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
According to the documentation, `Etag` should be included in the common response headers. However, it's missing in the examples. It seems we must check this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Ziy1-Tan (via GitHub)" <gi...@apache.org>.
Ziy1-Tan commented on PR #5710:
URL: https://github.com/apache/opendal/pull/5710#issuecomment-2710835479
> @Ziy1-Tan are you interested to fix these issues ? it's also better to test them locally.
Sure, Let me take a look.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Ziy1-Tan (via GitHub)" <gi...@apache.org>.
Ziy1-Tan commented on PR #5710:
URL: https://github.com/apache/opendal/pull/5710#issuecomment-2708152629
cc @meteorgan, would you mind taking a look at this PR? :)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo merged PR #5710:
URL: https://github.com/apache/opendal/pull/5710
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #5710:
URL: https://github.com/apache/opendal/pull/5710#issuecomment-2708372200
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986124152
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
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.
##########
core/src/services/cos/writer.rs:
##########
@@ -139,10 +154,11 @@ impl oio::MultipartWrite for CosWriter {
.cos_complete_multipart_upload(&self.path, upload_id, parts)
.await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
It looks like cos returns `ETag` in the `CompleteMultipartUploadResult` instead of header.
https://cloud.tencent.com/document/product/436/7742
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986317214
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
It seems COS may return `null` as the `version id` when it's `appendable` object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986310772
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
[In this document](https://cloud.tencent.com/document/product/436/7741), the response includes an `Etag`, but there is no `version_id`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986310772
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
[In this document](https://cloud.tencent.com/document/product/436/7741), the response includes an `Etag` in the response, but there is no `version_id`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986325684
##########
core/src/services/cos/writer.rs:
##########
@@ -139,10 +154,11 @@ impl oio::MultipartWrite for CosWriter {
.cos_complete_multipart_upload(&self.path, upload_id, parts)
.await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
It's the docs you referenced ealier.
<img width="918" alt="image" src="https://github.com/user-attachments/assets/08deda95-8713-4588-a08d-3ee68a69c46b" />
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986324007
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
Yep, cos doesn't support versioning for appendable object.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "meteorgan (via GitHub)" <gi...@apache.org>.
meteorgan commented on PR #5710:
URL: https://github.com/apache/opendal/pull/5710#issuecomment-2708877656
@Ziy1-Tan are you interested to fix these issues ? it's also better to test them locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986125655
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
Seems we are using the cos's non-public API: https://github.com/tencentyun/cos-go-sdk-v5/blob/e2423bebcf86fbf162ef04cf8c46b186ff05f593/object.go#L814-L827
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] services/cos: Implement Write Returns Metadata for cos [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #5710:
URL: https://github.com/apache/opendal/pull/5710#discussion_r1986125733
##########
core/src/services/cos/writer.rs:
##########
@@ -193,10 +209,11 @@ impl oio::AppendWrite for CosWriter {
let resp = self.core.send(req).await?;
+ let meta = Self::parse_metadata(resp.headers())?;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@opendal.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org