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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
