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

RFC-5495: List With Deleted #5495

Merged
merged 4 commits into from
Jan 2, 2025
Merged

RFC-5495: List With Deleted #5495

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 #5475

Rationale for this change

Add a new RFC for list with deleted.

What changes are included in this PR?

A new RFC

Are there any user-facing changes?

None.

Signed-off-by: Xuanwo <[email protected]>
@Xuanwo Xuanwo changed the title RFC: List With Deleted RFC-5495: List With Deleted Jan 2, 2025
@Xuanwo Xuanwo requested review from meteorgan and hoslo January 2, 2025 06:15
Signed-off-by: Xuanwo <[email protected]>
Copy link
Member

@tisonkun tisonkun left a comment

Choose a reason for hiding this comment

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

Rest LGTM. Two questions.

Comment on lines +35 to +36
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.

Copy link
Member

Choose a reason for hiding this comment

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

Then with_deleted or include_deleted?

Copy link
Member Author

Choose a reason for hiding this comment

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

Align with our existsing versions(true) API, we will have:

list_with(path).deleted(true).versions(true)

I think it's straightforward and clear enough.

Comment on lines +45 to +46
| `None` | `false` | **The metadata's associated file is not deleted, but its version status is either unknown or it is not the latest version.** This likely indicates that versioning is not enabled for this file, or versioning information is unavailable. |
| `None` | `true` | **The metadata's associated file is deleted, but its version status is either unknown or it is not the latest version.** This typically means the file was deleted without versioning enabled, or its versioning information is unavailable. This may represent an actual data deletion operation rather than an S3 delete marker. |
Copy link
Member

Choose a reason for hiding this comment

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

Seems deleted only relevant if versioning are enabled? If so, then these two variants seems not applicible.

Copy link
Member Author

@Xuanwo Xuanwo Jan 2, 2025

Choose a reason for hiding this comment

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

There are instances where we cannot determine if the metadata is current even when versioning has been enabled. For example, when calling HeadObject with a version ID to fetch the metadata of a path, we cannot tell whether it is current or not, though we do know whether it has been deleted. In such cases, its versioning information is unavailable (the None here).

@hoslo
Copy link
Contributor

hoslo commented Jan 2, 2025

LGTM.

@Xuanwo
Copy link
Member Author

Xuanwo commented Jan 2, 2025

LGTM.

Hi, please give an approve instead of a comment.

@Xuanwo Xuanwo merged commit c4cd89c into main Jan 2, 2025
263 checks passed
@Xuanwo Xuanwo deleted the list-with-deleted branch January 2, 2025 08:43
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.

3 participants