-
Notifications
You must be signed in to change notification settings - Fork 350
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
Parallelize S3 delete objects requests #5535
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
some non-blocking observations on lifetime&async, and atomic ordering
|
||
impl CancellationToken { | ||
fn cancel(&self) { | ||
self.0.store(true, Ordering::Release); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this access can be Relaxed, we're only interested in how the memory address changes over time, not to how its changes relate to changes in the rest of the memory of our program
} | ||
|
||
fn is_cancelled(&self) -> bool { | ||
self.0.load(Ordering::Acquire) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same wrt Relaxed
let bucket = self.bucket.clone(); | ||
let retry_params = self.retry_params; | ||
let cancellation_token = cancellation_token.clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because of how this code is written (structured concurrency vs the more usual tokio::spawn), bucket and cancellation_token need not be cloned, refs are enough (and CancellationToken could be made to not have an inner Arc)
i don't know if it should be done that way, but it certainly can
This PR is no longer necessary and since the patch is complex, I'd rather close. |
Description
Parallelize S3 delete objects requests
How was this PR tested?
Ran
quickwit-storage
test suite locally.