Add mechanism to cancel the operation of uploading directory #75
+502
−111
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
2 tasks
aajtodd
reviewed
Nov 20, 2024
This commit addresses #75 (comment)
This commit addresses #75 (comment)
This commit addresses #75 (comment)
aajtodd
approved these changes
Nov 22, 2024
waahm7
reviewed
Nov 25, 2024
Comment on lines
+43
to
+46
| /// In contrast, calling [`Self::abort`] attempts to cancel ongoing tasks more explicitly. | ||
| /// It first calls `.abort_all` on the tasks it owns, and then invokes `AbortMultipartUpload` | ||
| /// to abort any in-progress multipart uploads. Errors encountered during `AbortMultipartUpload` | ||
| /// are logged, but do not affect the overall cancellation flow. |
There was a problem hiding this comment.
trivial: Worth mentioning that the operation might already be complete, and we won't delete anything?
| @@ -18,13 +42,61 @@ pub struct UploadObjectsHandle { | |||
|
|
|||
| impl UploadObjectsHandle { | |||
| /// Consume the handle and wait for the upload to complete | |||
| /// | |||
| /// When the `FailedTransferPolicy` is set to [`FailedTransferPolicy::Abort`], this method | |||
| /// will return an error if any of the spawned tasks encounter one. The other tasks will | |||
There was a problem hiding this comment.
Suggested change
| /// will return an error if any of the spawned tasks encounter one. The other tasks will | |
| /// will return the first error if any of the spawned tasks encounter one. The other tasks will |
| @@ -617,4 +703,35 @@ mod unit { | |||
| ); | |||
| } | |||
| } | |||
|
|
|||
| #[tokio::test] | |||
There was a problem hiding this comment.
I think we should add a test that tries to do MPU but fails and calls abortMultipart instead of completeMultipart.
There was a problem hiding this comment.
Good point. As discussed offline, a good place to add that test is probably an integration test file for uploading single object.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available:
Resolves a subtask in #65
Description of changes:
This PR introduces a mechanism to cancel the upload of multiple objects within a directory. It distinguishes between aborting and dropping a handle for both
UploadHandleandUploadObjectsHandle. Note that this PR does not modify theUploadHandlefunctionality, except for adding some comments to the struct.For both of the handles, the basic idea is
.drop()is called explicitly. The tasks will be canceled at the points where their futures are waiting, and we don't have much control over the cleanup process, which is left to the underlying async runtime..abort()on it) allows tasks to complete their current work while ignoring any future tasks. The word "complete" has different meanings depending on whether the handle is anUploadHandleor anUploadObjectsHandle.UploadHandle, it means that the control flow attempts to abort ongoing read and upload tasks, and it will invokeAbortMultipartUploadin the case of MPU.UploadObjectsHandle, it means that each upload task either finishes uploading the current object or invokes.abort()on its respective single upload operation down the line.For
UploadObjectsHandle, this PR introduces a watch channel that signals when the operation enters cancellation mode and needs to be terminated gracefully. There are two ways to trigger cancellation:.abort()on theUploadObjectsHandle.Both cases are covered by newly added integration tests.
Design Discussions:
Dropthe same as calling.abort()—for example, having fndrop(&mut self)call.abort(). However, since.dropis a synchronous method and.abort()is asynchronous, this led me to implement separate behaviors for the two..abort()(for bothUploadHandleandUploadObjectsHandle) will only return the error that triggered the cancellation and will log any additional errors encountered along the way (e.g., errors from AbortMultipartUpload). We have TODO and I also left TODO to assess this behavior, but for now, the code after this PR "swallows" and logs any new errors encountered during the cancellation process.By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.