Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(core): add version(bool) for List operation to include version d… #5106

Merged
merged 8 commits into from
Sep 19, 2024

Conversation

meteorgan
Copy link
Contributor

@meteorgan meteorgan commented Sep 9, 2024

Which issue does this PR close?

part of #2611.

Rationale for this change

What changes are included in this PR?

  1. add version(bool) for List operation
  2. support version for s3 List

Are there any user-facing changes?

@meteorgan
Copy link
Contributor Author

meteorgan commented Sep 9, 2024

There are a few issues to discuss:

  1. since we can retrieve the bucket versioning status using GetBucketVersioning API, should we use this API to set bucket_versioning_enabled, and avoid requiring users to pass this parameter manually ? The downside is that doing so would require call the API in Builder which might negatively affect users who don't need versioning. Additionally, the GetBucketVersioning API requires bucket owner permissions, while ListObjectVersions only requires READ access to the bucket.

@meteorgan
Copy link
Contributor Author

  1. Because list and list with version use different APIs in S3, should we add new Github actions for S3, such as aws_s3_with_version ? if we only use a bucket with object versioning enabled to test all cases, we might unintentionally skip the tests for ListObjectsV2

@meteorgan
Copy link
Contributor Author

  1. Since ListObjectVersions doesn't has start_after parameter, but instead uses key-marker and version-id-marker. should we set key-marker to start_after ? However, according to the document, key-marker: Specifies the key to start with when listing objects in a bucket, it seems to be inclusive. In contrast, the start-after parameter in ListObjectsV2 is exclusive.

@meteorgan
Copy link
Contributor Author

  1. Since ListObjectVersions doesn't has start_after parameter, but instead uses key-marker and version-id-marker. should we set key-marker to start_after ? However, according to the document, key-marker: Specifies the key to start with when listing objects in a bucket, it seems to be inclusive. In contrast, the start-after parameter in ListObjectsV2 is exclusive.

But I've found some sources suggesting that key-marker is actually exclusive(for example: this discussion). I'll verify it later to confirm.

@Xuanwo
Copy link
Member

Xuanwo commented Sep 9, 2024

  1. since we can retrieve the bucket versioning status using GetBucketVersioning API, should we use this API to set bucket_versioning_enabled, and avoid requiring users to pass this parameter manually ? The downside is that doing so would require call the API in Builder which might negatively affect users who don't need versioning. Additionally, the GetBucketVersioning API requires bucket owner permissions, while ListObjectVersions only requires READ access to the bucket.

We are not permitted to call the async API during the build stage, so we can't call the API there. I prefer to have an enable_version option for the user to set. Only after users set enable_version will we enable the related capability. This also addresses testing concerns: we can add a new test action with OPENDA_S3_ENABLE_VERSION.

3. Since ListObjectVersions doesn't has start_after parameter, but instead uses key-marker and version-id-marker. should we set key-marker to start_after ?

Yes, I believe we can use start_with as users key-marker.

core/src/raw/oio/list/page_list.rs Outdated Show resolved Hide resolved
core/src/services/s3/backend.rs Outdated Show resolved Hide resolved
core/src/services/s3/backend.rs Outdated Show resolved Hide resolved
core/src/services/s3/lister.rs Show resolved Hide resolved
@meteorgan
Copy link
Contributor Author

Ceph differs from S3 in its handling of the key-marker parameter. in Ceph, it's inclusive, which is why this check is failing.
How should we address this issue ? @Xuanwo

@Xuanwo
Copy link
Member

Xuanwo commented Sep 12, 2024

Ceph differs from S3 in its handling of the key-marker parameter. in Ceph, it's inclusive, which is why this check is failing. How should we address this issue ? @Xuanwo

Would you like to submit an issue for rados? As for now, we can remove the versioning test for rados first.

@meteorgan
Copy link
Contributor Author

Ceph differs from S3 in its handling of the key-marker parameter. in Ceph, it's inclusive, which is why this check is failing. How should we address this issue ? @Xuanwo

Would you like to submit an issue for rados? As for now, we can remove the versioning test for rados first.

https://tracker.ceph.com/issues/68055 (The Issues of Ceph repository is not enabled)

@Xuanwo
Copy link
Member

Xuanwo commented Sep 13, 2024

tracker.ceph.com/issues/68055 (The Issues of Ceph repository is not enabled)

We can create an issue on the OpenDAL side to keep track of it.

@meteorgan meteorgan marked this pull request as ready for review September 13, 2024 11:05
@meteorgan
Copy link
Contributor Author

tracker.ceph.com/issues/68055 (The Issues of Ceph repository is not enabled)

We can create an issue on the OpenDAL side to keep track of it.

Ok, Let me do it later

Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for your work, @meteorgan!

@Xuanwo Xuanwo merged commit 9313561 into apache:main Sep 19, 2024
231 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants