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): Implement list with deleted for s3 service #5498

Merged
merged 4 commits into from
Jan 2, 2025

Conversation

Xuanwo
Copy link
Member

@Xuanwo Xuanwo commented Jan 2, 2025

Which issue does this PR close?

Part of #5496

Rationale for this change

Implement the RFC we need.

What changes are included in this PR?

  • Add deleted in OpList
  • Add is_deleted in Metadata
  • Implement tests for list with deleted
  • Implement list with deleted for s3 services

Are there any user-facing changes?

Users can now list deleted files from s3 services.

Copy link

codspeed-hq bot commented Jan 2, 2025

CodSpeed Performance Report

Merging #5498 will improve performances by 18.88%

Comparing implement-list-with-deleted (a739a68) with main (7a83013)

Summary

⚡ 4 improvements
✅ 69 untouched benchmarks

Benchmarks breakdown

Benchmark main implement-list-with-deleted Change
256 KiB * 1000k chunk 183.6 ns 154.4 ns +18.88%
256 KiB * 100k chunk 183.6 ns 154.4 ns +18.88%
256 KiB * 10k chunk 183.6 ns 154.4 ns +18.88%
256 KiB * 1k chunk 212.8 ns 183.6 ns +15.89%

Signed-off-by: Xuanwo <[email protected]>
Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Xuanwo requested review from meteorgan, hoslo and dqhl76 January 2, 2025 13:12
Copy link
Contributor

@hoslo hoslo left a comment

Choose a reason for hiding this comment

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

LGTM.

@Xuanwo Xuanwo merged commit 0146a12 into main Jan 2, 2025
277 checks passed
@Xuanwo Xuanwo deleted the implement-list-with-deleted branch January 2, 2025 13:27
if let Some(etag) = version_object.etag {
meta.set_etag(&etag);
meta.set_content_md5(etag.trim_matches('"'));
if self.args.deleted() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Did I misunderstand something ? this implementation seems to differ from the RFC description:

Please note that `deleted` here means "including deleted files" rather than "only deleted files." Therefore, `list_with(path).deleted(true)` will list both current files and deleted ones.

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.

3 participants