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

test(core): add tests for versioning #5132

Merged
merged 6 commits into from
Sep 25, 2024
Merged

Conversation

meteorgan
Copy link
Contributor

@meteorgan meteorgan commented Sep 21, 2024

Which issue does this PR close?

part of: #2611

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

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

In #5106, I didn't address the DeleteMarker in the response. However, it could be useful for the users who want to know which objects haven been deleted. Should we return the DeleteMarker as a object version with a delete_marker tag when performing a list_with_version request ?

@Xuanwo
Copy link
Member

Xuanwo commented Sep 23, 2024

In #5106, I didn't address the DeleteMarker in the response. However, it could be useful for the users who want to know which objects haven been deleted. Should we return the DeleteMarker as a object version with a delete_marker tag when performing a list_with_version request ?

I believe they should be included when users have list_with(path).deleted(true). Would you like to start another issue to track this?

core/src/types/capability.rs Outdated Show resolved Hide resolved
core/tests/behavior/async_delete.rs Outdated Show resolved Hide resolved
@meteorgan
Copy link
Contributor Author

In #5106, I didn't address the DeleteMarker in the response. However, it could be useful for the users who want to know which objects haven been deleted. Should we return the DeleteMarker as a object version with a delete_marker tag when performing a list_with_version request ?

I believe they should be included when users have list_with(path).deleted(true). Would you like to start another issue to track this?

Ok. And I want to discuss some additional details about this issue.

@meteorgan
Copy link
Contributor Author

If versioning is not enabled, should we return ErrorKind::Unsupported for every operation with version parameter ? These cases weren't addressed in earlier PR, such as: #4873

@Xuanwo
Copy link
Member

Xuanwo commented Sep 24, 2024

If versioning is not enabled, should we return ErrorKind::Unsupported for every operation with version parameter ? These cases weren't addressed in earlier PR, such as: #4873

I believe we should make our best effort to fetch them since it doesn't affect the result. It's fine to attempt fetching even if versioning isn't enabled. The situation is different with write_with_if_none_match, which may lead to incorrect behavior like writing data which wrong condition.

core/src/services/s3/backend.rs Outdated Show resolved Hide resolved
core/src/services/s3/error.rs Outdated Show resolved Hide resolved
@meteorgan
Copy link
Contributor Author

If versioning is not enabled, should we return ErrorKind::Unsupported for every operation with version parameter ? These cases weren't addressed in earlier PR, such as: #4873

I believe we should make our best effort to fetch them since it doesn't affect the result. It's fine to attempt fetching even if versioning isn't enabled. The situation is different with write_with_if_none_match, which may lead to incorrect behavior like writing data which wrong condition.

what about list ? in #5106 (comment), we decided to return error when versioning is not enabled.

@Xuanwo
Copy link
Member

Xuanwo commented Sep 24, 2024

what about list ? in #5106 (comment), we decided to return error when versioning is not enabled.

Well, that makes sense. Let's return unsupported for all those cases.

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.

Perfect! Thanks a lot.

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

Successfully merging this pull request may close these issues.

2 participants