You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@opendal.apache.org by "suyanhanx (via GitHub)" <gi...@apache.org> on 2024/01/08 15:23:23 UTC
[PR] feat(services/s3): object versioning [incubator-opendal]
suyanhanx opened a new pull request, #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943
(no comment)
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1446123980
##########
core/src/services/s3/core.rs:
##########
@@ -664,7 +682,7 @@ impl S3Core {
pub async fn s3_delete_objects(
&self,
- paths: Vec<String>,
+ paths: Vec<(String, Option<String>)>,
Review Comment:
> It's ok to keep it as is.
Let's keep batch api as is.
--
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] feat(services/s3): Add object versioning support [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3943:
URL: https://github.com/apache/opendal/pull/3943#issuecomment-2144122620
> Thanks. Any update?
Thanks for the inquery. I will working on this after 0.47 released (about this week).
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#issuecomment-1885240908
> But as what @Xuanwo said, we don't need to support multiple versions for now.
This is incorrect.
I'm not saying *we don't need to support multiple versions for now*. What I meant is that we should consider `version` as part of our public API and test it within the same behavioral test suites.
> If we want to support multiple version for S3 service, I think we need support list/(get the object by version) at the same time. Otherwise the API would be not symmetry if we just add it for delete api
Thay are already added in our [Object Version](https://opendal.apache.org/docs/rust/opendal/docs/rfcs/rfc_2602_object_versioning/index.html) RFC.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#issuecomment-1885230200
The code is OK.
But as what @Xuanwo said, we don't need to support multiple versions for now.
If we want to support multiple version for S3 service, I think we need support list/(get the object by version) at the same time. Otherwise the API would be not symmetry if we just add it for delete api
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1446142959
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
> Is it possible for us to set up some additional composite actions for those cases where we need to add special settings to the service itself?
It's an existing nature that some s3 bucket doesn't enable object versioning. So the steps we need to take is:
- Add read_with_version, stat_with_version capabilities.
- Add enable_version for s3 service. We will enable corresponding capabilities if this flag has been enabled.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1447253638
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
> We could keep them for just 1 day.
> Could the test bucket we're currently using be configured the same way?
Yes, already have.
--
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] feat(services/s3): Add object versioning support [opendal]
Posted by "drernie (via GitHub)" <gi...@apache.org>.
drernie commented on PR #3943:
URL: https://github.com/apache/opendal/pull/3943#issuecomment-2135778274
Thanks. Any update?
> On Mar 13, 2024, at 20:55, Suyan ***@***.***> wrote:
>
>
> Is this PR still under development? We are very interested in using OpenDAL for reading and writing versioned files on S3 buckets, and it looks like we can't do that until this PR is merged.
>
> We would be delighted to help you with testing!
>
> Thank you for your attention! I'm still working on the development. I will finish it as soon as possible.
>
> —
> Reply to this email directly, view it on GitHub <https://github.com/apache/opendal/pull/3943#issuecomment-1996350596>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAE2T6DX7E23RS6OP4SIKDYYENT3AVCNFSM6AAAAABBRUC7YSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJWGM2TANJZGY>.
> You are receiving this because you commented.
>
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1446119392
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
It's an extra burden for other unrelated tests, and I think it's unnecessary. 🤔
So I put this in the edge tests.
(Of course, a new s3 setup is necessary, wherever it is put.)
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1446137501
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
Is it possible for us to set up some additional composite actions for those cases where we need to add special settings to the service itself?
Agree that setting up one by one is cumbersome.
But for now, we could set it first.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1447243193
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
> I'm mainly considering the cleanup issue. With storage services that have version control, cleanup can be more complicated.
Storage services have version support will also have `lifecycle`. This should not be an issue.
--
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] feat(services/s3): Add object versioning support [opendal]
Posted by "drernie (via GitHub)" <gi...@apache.org>.
drernie commented on PR #3943:
URL: https://github.com/apache/opendal/pull/3943#issuecomment-1996164410
Is this PR still under development? We are very interested in using OpenDAL for reading and writing versioned files on S3 buckets, and it looks like we can't do that until this PR is merged.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1447185154
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
I'm mainly considering the cleanup issue. With storage services that have version control, cleanup can be more complicated. We'd have to remove every version of each file, so I hope to minimize the scope of the impact as much as possible. Since version control should not affect normal user usage, there should be no need to test the functionality of version control unless it is enabled.
So, can we:
1. Add version-related test cases to the existing behavior test. In this way, we can use environment variables to skip cases that do not involve version control, based on whether version control is enabled. But it also needs to add some extra cleanup steps.
1. Create a separate behavior test suite(maybe like this PR, but not in edge tests) that does not affect existing tests.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1446120744
##########
core/src/services/s3/core.rs:
##########
@@ -664,7 +682,7 @@ impl S3Core {
pub async fn s3_delete_objects(
&self,
- paths: Vec<String>,
+ paths: Vec<(String, Option<String>)>,
Review Comment:
Deleter hasn't been implemented yet, so I changed the batch.
It's ok to keep it as is.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1444905983
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
This should be part of behavior test instead of edge test. We can add a new s3 setup with version enabled.
##########
core/src/services/s3/core.rs:
##########
@@ -664,7 +682,7 @@ impl S3Core {
pub async fn s3_delete_objects(
&self,
- paths: Vec<String>,
+ paths: Vec<(String, Option<String>)>,
Review Comment:
We don't need to support delete multiple versions for now. This should be considered in deleter.
--
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] feat(services/s3): Add object versioning support [incubator-opendal]
Posted by "Zheaoli (via GitHub)" <gi...@apache.org>.
Zheaoli commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1447705347
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
> I'm mainly considering the cleanup issue. With storage services that have version control, cleanup can be more complicated. We'd have to remove every version of each file, so I hope to minimize the scope of the impact as much as possible. Since version control should not affect normal user usage, there should be no need to test the functionality of version control unless it is enabled.
>
> So, can we:
>
> 1. Add version-related test cases to the existing behavior test. In this way, we can use environment variables to skip cases that do not involve version control, based on whether version control is enabled. But it also needs to add some extra cleanup steps.
> 2. Create a separate behavior test suite(maybe like this PR, but not in edge tests) that does not affect existing tests.
The test infra would not be a problem we just need to configure the lifecycle like the following below
--
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] feat(services/s3): Add object versioning support [opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on PR #3943:
URL: https://github.com/apache/opendal/pull/3943#issuecomment-1996350596
> Is this PR still under development? We are very interested in using OpenDAL for reading and writing versioned files on S3 buckets, and it looks like we can't do that until this PR is merged.
>
> We would be delighted to help you with testing!
Thank you for your attention! I'm still working on the development. I will finish it as soon as possible.
--
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] feat(services/s3): Add object versioning support [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on PR #3943:
URL: https://github.com/apache/opendal/pull/3943#issuecomment-2219824474
Has been implemented by https://github.com/apache/opendal/pull/4873. Thanks @suyanhanx's effort.
--
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] feat(services/s3): Add object versioning support [opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo closed pull request #3943: feat(services/s3): Add object versioning support
URL: https://github.com/apache/opendal/pull/3943
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1446123295
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
> It's an extra burden for other unrelated tests, and I think it's unnecessary. 🤔
It's our public API that many services could implement. Adding edge tests for them one by one adds more burden.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "Xuanwo (via GitHub)" <gi...@apache.org>.
Xuanwo commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1447250859
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
> We'd have to remove every version of each file, so I hope to minimize the scope of the impact as much as possible.
Please don't overestimate the *impact* of test artifacts. In the worst cases, we have some files left in a bucket which is fine.
> Since version control should not affect normal user usage
`version` is part of our public API. Please don't think of it's *not affect normal user usage*. It should be treated the same as `range`, `buffer`, `concurrent`. I see no reason to test `version` separately.
> there should be no need to test the functionality of version control unless it is enabled.
This is why we have `capability`.
---
> 1. Add version-related test cases to the existing behavior test. In this way, we can use environment variables to skip cases that do not involve version control, based on whether version control is enabled.
We don't require additional environment variables. Instead, we need a fresh setup for services. Take S3, for instance; we require a newly configured bucket with the feature enabled.
Additionally, we must introduce a new flag for the S3 service, which should be integrated into our API rather than configured in CI.
--
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] feat(services/s3): object versioning [incubator-opendal]
Posted by "suyanhanx (via GitHub)" <gi...@apache.org>.
suyanhanx commented on code in PR #3943:
URL: https://github.com/apache/incubator-opendal/pull/3943#discussion_r1447253098
##########
.github/workflows/edge_test.yml:
##########
@@ -137,3 +137,37 @@ jobs:
OPENDAL_S3_BUCKET: opendal-testing
OPENDAL_S3_ROLE_ARN: arn:aws:iam::952853449216:role/opendal-testing
OPENDAL_S3_REGION: ap-northeast-1
+
+ test_s3_on_versionning_enable_bucket:
Review Comment:
Wow, that's a great news.
We could keep them for just 1 day.
Could the test bucket we're currently using be configured the same way?
--
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