Skip to content

Conversation

rakshil14-2
Copy link

@rakshil14-2 rakshil14-2 commented Aug 3, 2025

Issue :#4070

Description of changes:
Added a flag for deleting all versions of an object present in a versioned-enable bucket. For dealing with versions, a list-object-versions api is called to get the version ids as well as delete markers of the objects. Once the object and their version ids are retrieved, then a batch of objects with their version id is created which is passed to delete-objects api for deletion.

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@rakshil14-2 rakshil14-2 marked this pull request as ready for review August 5, 2025 23:05
@aemous aemous requested a review from a team August 7, 2025 17:49
@rakshil14-2 rakshil14-2 changed the title Batch creation for delete Deleting all versions of objects Aug 7, 2025
Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

Let's also generate a changelog entry.

looks good overall, a few questions/requests

Copy link
Contributor

@aemous aemous left a comment

Choose a reason for hiding this comment

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

LGTM

Rakshil Modi added 2 commits August 15, 2025 11:55
Adding response to delete handler
Updated comments

Remove BucketLister class

Added change logs

Modifying S3 Handler

Format display message

Changing names of the class

changing to marked down
result_queue, result_recorder, result_processor
)

if self._requires_batch_handler():
Copy link
Member

@kdaily kdaily Aug 15, 2025

Choose a reason for hiding this comment

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

I'm not 100% sure about adding logic to change the type returned. In the function signature it says:

        :returns: A S3TransferHandler instance

Does a BatchS3TransferHandler act like the existing S3TransferHandler?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, but BatchS3TransferHandler will handler batching operations

@@ -0,0 +1,5 @@
{
"type": "enhancement",
"category": "`s3`",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I think this should have two ticks on both sides. ``s3``

cli_params,
)
# Submitter list for batch request submitters
self._batch_submitters = [
Copy link
Member

Choose a reason for hiding this comment

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

Could this be:

Suggested change
self._batch_submitters = [
self._submitters = [

to follow the same interface as the S3TransferHandler?

Comment on lines +728 to +729
:type versionedFileInfo: Set of VersionedFileInfo to submit to batch submitter
:param fileinfos: The fileinfos to submit to the batch submitter
Copy link
Member

Choose a reason for hiding this comment

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

I think keeping the same structure as the existing handler would be preferrable to describe, since the only underlying changes are the type of iterable and the submitters being called on them.

Suggested change
:type versionedFileInfo: Set of VersionedFileInfo to submit to batch submitter
:param fileinfos: The fileinfos to submit to the batch submitter
:type fileinfos: iterable of VersionedFileInfos
param fileinfos: Set of VersionedFileInfos to submit to underlying transfer
request submitters to make transfer API calls to S3

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