alamb commented on code in PR #524:
URL:
https://github.com/apache/arrow-rs-object-store/pull/524#discussion_r2478448111
##########
src/lib.rs:
##########
@@ -873,9 +873,25 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync +
Debug + 'static {
/// Delete all the objects at the specified locations
///
/// When supported, this method will use bulk operations that delete more
- /// than one object per a request. The default implementation will call
- /// the single object delete method for each location, but with up to 10
- /// concurrent requests.
+ /// than one object per a request. Otherwise, the implementation may call
+ /// the single object delete method for each location. For example, the
+ /// following implementation deletes each object with up to 10 concurrent
+ /// requests:
+ ///
+ /// ```ignore
+ /// let client = Arc::clone(&self.client);
+ /// locations
+ /// .map(move |location| {
+ /// let client = Arc::clone(&client);
Review Comment:
👍
##########
src/lib.rs:
##########
@@ -873,9 +873,25 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync +
Debug + 'static {
/// Delete all the objects at the specified locations
///
/// When supported, this method will use bulk operations that delete more
- /// than one object per a request. The default implementation will call
- /// the single object delete method for each location, but with up to 10
- /// concurrent requests.
+ /// than one object per a request. Otherwise, the implementation may call
+ /// the single object delete method for each location. For example, the
+ /// following implementation deletes each object with up to 10 concurrent
+ /// requests:
+ ///
+ /// ```ignore
Review Comment:
Could you please make this an actual example that compiles (so we are sure
it won't drift over time)?
Perhaps following the model f the examples below.
We can do this as a follow on PR
##########
src/lib.rs:
##########
@@ -873,9 +873,25 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync +
Debug + 'static {
/// Delete all the objects at the specified locations
///
/// When supported, this method will use bulk operations that delete more
- /// than one object per a request. The default implementation will call
- /// the single object delete method for each location, but with up to 10
- /// concurrent requests.
+ /// than one object per a request. Otherwise, the implementation may call
Review Comment:
We should probably call this out as a behavior change too in the API docs
(but I think it is better to avoid implementing concurrent requests by default)
Also I think the documentation to explain how to get the old behavior is good
##########
src/throttle.rs:
##########
@@ -237,6 +237,19 @@ impl<T: ObjectStore> ObjectStore for ThrottledStore<T> {
self.inner.delete(location).await
}
+ fn delete_stream(
Review Comment:
I wonder if we should add a test for this new implementation, following the
other tests at this file
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
