You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "linhr (via GitHub)" <gi...@apache.org> on 2025/10/30 10:29:29 UTC
[PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
linhr opened a new pull request, #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524
# Which issue does this PR close?
Closes #518.
# Rationale for this change
The current `delete_stream` method looks like this:
```rust
fn delete_stream<'a>(
&'a self,
locations: BoxStream<'a, Result<Path>>,
) -> BoxStream<'a, Result<Path>>;
```
This PR changes the `delete_stream` method to work with `'static` lifetime. The reason for this change was discussed in the corresponding issue #518.
# What changes are included in this PR?
As I work on this PR, the changes appeared to be more complex than thought. The challenge is that `delete_stream` has a default implementation that takes `&self`, so the returned stream cannot be `'static`. I've considered the following options for this challenge.
**Option 1**: Only change the input stream to be `'static` without changing the lifetime of the output stream. The method would look like this:
```rust
fn delete_stream<'a>(
&'a self,
locations: BoxStream<'static, Result<Path>>,
) -> BoxStream<'a, Result<Path>>;
```
Or equivalently (with unnecessary lifetime elided):
```rust
fn delete_stream(
&self,
locations: BoxStream<'static, Result<Path>>,
) -> BoxStream<'_, Result<Path>>;
```
This would result in minimum code change but the result is still not very elegant, since we're not fully migrating to `BoxStream<'static, _>`.
**Option 2**: Change both the input and output stream to be `'static` and **requires the implementation to define `delete_stream` explicitly** (i.e. no more default implementation). The method would look like this:
```rust
fn delete_stream(
&self,
locations: BoxStream<'static, Result<Path>>,
) -> BoxStream<'static, Result<Path>>;
```
I ended up choosing **Option 2** for this PR. This is more work but gives a clean end result so that **all `ObjectStore` methods now handle input/output `BoxStream` of `'static` lifetime**.
And fortunately, it likely won't be challenging for downstream implementations even if `delete_stream` is now **required**. Most implementations already use `Arc` for internal states, which can be cheaply cloned and moved into the `'static` output stream. From the changes I made to various `ObjectStore` implementations in this PR, we can see that the logic is straightforward. And I've documented the pattern in `lib.rs` for downstream implementations to follow.
# Are there any user-facing changes?
This is a breaking change that could be considered for 0.13 (#367).
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb merged PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "linhr (via GitHub)" <gi...@apache.org>.
linhr commented on code in PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#discussion_r2480416712
##########
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:
Done!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "kylebarron (via GitHub)" <gi...@apache.org>.
kylebarron commented on code in PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#discussion_r2481745713
##########
src/lib.rs:
##########
@@ -873,9 +873,8 @@ 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
Review Comment:
It would be nice as an end user to know which backends (at least of those provided by object-store) support bulk delete
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "linhr (via GitHub)" <gi...@apache.org>.
linhr commented on PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#issuecomment-3471074428
Thanks @alamb for the review! I feel I can update the docs and tests in the same PR. I'll work on this today!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#discussion_r2482597389
##########
src/lib.rs:
##########
@@ -927,6 +910,119 @@ pub trait ObjectStore: std::fmt::Display + Send + Sync + Debug + 'static {
/// # let rt = tokio::runtime::Builder::new_current_thread().build().unwrap();
/// # rt.block_on(example()).unwrap();
/// ```
+ ///
+ /// Note: Before version 0.13, `delete_stream` has a default implementation
+ /// that deletes each object with up to 10 concurrent requests. This default
+ /// behavior has been removed, and each implementation must now provide its
+ /// own `delete_stream` implementation explicitly. The following example
+ /// shows how to implement `delete_stream` to get the previous default
Review Comment:
❤️
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "linhr (via GitHub)" <gi...@apache.org>.
linhr commented on PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#issuecomment-3467262309
The changes are covered by existing tests since `delete_stream` is used in `src/integration.rs`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "linhr (via GitHub)" <gi...@apache.org>.
linhr commented on code in PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#discussion_r2480420244
##########
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:
Done!
I called out the behavior change in the docs and mentioned that the example can be used to get the previous behavior.
##########
src/throttle.rs:
##########
@@ -237,6 +237,19 @@ impl<T: ObjectStore> ObjectStore for ThrottledStore<T> {
self.inner.delete(location).await
}
+ fn delete_stream(
Review Comment:
Done!
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on code in PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#discussion_r2482595727
##########
src/lib.rs:
##########
@@ -873,9 +873,8 @@ 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
Review Comment:
- Filed https://github.com/apache/arrow-rs-object-store/issues/527
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat!: use `'static` lifetime for `delete_stream` [arrow-rs-object-store]
Posted by "alamb (via GitHub)" <gi...@apache.org>.
alamb commented on PR #524:
URL: https://github.com/apache/arrow-rs-object-store/pull/524#issuecomment-3474800518
Thanks for the review @kylebarron
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org