Skip to content

Comments

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

Merged
Xuanwo merged 8 commits intoapache:mainfrom
meteorgan:list_with_version
Sep 19, 2024
Merged

feat(core): add version(bool) for List operation to include version d…#5106
Xuanwo merged 8 commits intoapache:mainfrom
meteorgan:list_with_version

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?

@github-actions github-actions bot added the releases-note/feat The PR implements a new feature or has a title that begins with "feat" label Sep 9, 2024
@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.

@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 meteorgan requested a review from PsiACE as a code owner 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
@meteorgan
Copy link
Contributor Author

Thanks a lot for your work, @meteorgan!

There's one thing to note: I haven't added versioning behavior test for AWS. it seems like only you can do that ?

@Xuanwo
Copy link
Member

Xuanwo commented Sep 20, 2024

There's one thing to note: I haven't added versioning behavior test for AWS. it seems like only you can do that ?

I can enable the version for bucket, would you like to add a new workflow for it?

@meteorgan
Copy link
Contributor Author

There's one thing to note: I haven't added versioning behavior test for AWS. it seems like only you can do that ?

I can enable the version for bucket, would you like to add a new workflow for it?

Ok. I'll add it later

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/feat The PR implements a new feature or has a title that begins with "feat"

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants