feat(services/s3): Add object versioning support #3943

suyanhanx · 2024-01-08T15:23:21Z

Part of #2611

Xuanwo · 2024-01-08T16:04:00Z

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


This should be part of behavior test instead of edge test. We can add a new s3 setup with version enabled.

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

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.

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.

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.

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.

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.

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.

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?

We could keep them for just 1 day.
Could the test bucket we're currently using be configured the same way?

Yes, already have.

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:

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.

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

Xuanwo · 2024-01-08T16:04:45Z

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>)>,


We don't need to support delete multiple versions for now. This should be considered in deleter.

Deleter hasn't been implemented yet, so I changed the batch.
It's ok to keep it as is.

It's ok to keep it as is.

Let's keep batch api as is.

Zheaoli · 2024-01-10T16:53:05Z

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

Xuanwo · 2024-01-10T16:59:35Z

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

drernie · 2024-03-14T00:08:51Z

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!

suyanhanx · 2024-03-14T03:55:20Z

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.

drernie · 2024-05-28T17:32:56Z

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 <#3943 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAE2T6DX7E23RS6OP4SIKDYYENT3AVCNFSM6AAAAABBRUC7YSVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSOJWGM2TANJZGY>. You are receiving this because you commented.

github-actions bot added the releases-note/feat label Jan 8, 2024

Xuanwo reviewed Jan 8, 2024

View reviewed changes

Xuanwo changed the title ~~feat(services/s3): object versioning~~ feat(services/s3): Add object versioning support Jan 10, 2024

suyanhanx force-pushed the object-versioning branch from 33428b2 to 8a2d2f1 Compare January 11, 2024 13:59

suyanhanx added 2 commits March 15, 2024 19:55

feat

e6bf657

async tests

ebf1058

suyanhanx force-pushed the object-versioning branch from 8a2d2f1 to ebf1058 Compare March 18, 2024 07:52

jcsp mentioned this pull request Apr 30, 2024

pageserver: adopt opendal? neondatabase/neon#7548

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(services/s3): Add object versioning support #3943

feat(services/s3): Add object versioning support #3943

suyanhanx commented Jan 8, 2024 •

edited by Xuanwo

Xuanwo Jan 8, 2024

suyanhanx Jan 9, 2024

Xuanwo Jan 9, 2024

suyanhanx Jan 9, 2024

Xuanwo Jan 9, 2024

Xuanwo Jan 10, 2024

Xuanwo Jan 10, 2024

suyanhanx Jan 10, 2024

Xuanwo Jan 10, 2024

Zheaoli Jan 10, 2024

Xuanwo Jan 8, 2024

suyanhanx Jan 9, 2024

Xuanwo Jan 9, 2024

Zheaoli commented Jan 10, 2024

Xuanwo commented Jan 10, 2024

drernie commented Mar 14, 2024 •

edited

suyanhanx commented Mar 14, 2024

drernie commented May 28, 2024 via email

feat(services/s3): Add object versioning support #3943

Are you sure you want to change the base?

feat(services/s3): Add object versioning support #3943

Conversation

suyanhanx commented Jan 8, 2024 • edited by Xuanwo

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

Choose a reason for hiding this comment

Zheaoli commented Jan 10, 2024

Xuanwo commented Jan 10, 2024

drernie commented Mar 14, 2024 • edited

suyanhanx commented Mar 14, 2024

drernie commented May 28, 2024 via email

suyanhanx commented Jan 8, 2024 •

edited by Xuanwo

drernie commented Mar 14, 2024 •

edited