Remove Nested async and Fallibility from ObjectStore::list #4930
Conversation
| @@ -1158,21 +1156,14 @@ mod tests { | |||
|
|
|||
| let store = LocalFileSystem::new_with_prefix(root.path()).unwrap(); | |||
|
|
|||
| // `list` must fail | |||
There was a problem hiding this comment.
We no longer have this peculiar arrangement where an error can emerge in two different places
| let result: Vec<_> = integration | ||
| .list(prefix) | ||
| .await | ||
| .unwrap() | ||
| .try_collect() | ||
| .await | ||
| .unwrap(); | ||
| let result: Vec<_> = integration.list(prefix).try_collect().await.unwrap(); |
There was a problem hiding this comment.
This is much nicer imo
object_store/src/limit.rs
Outdated
| let s = self.inner.list_with_offset(prefix, offset); | ||
| let fut = Arc::clone(&self.semaphore) |
There was a problem hiding this comment.
This somewhat peculiar ordering is required because the returned stream cannot borrow prefix, only self
tustvold
force-pushed
the
list-nested-async
branch
from
3246471 to
f4dcb53
Compare
|
|
I think this means we need to release another SemVer change for object_store on the next release |
Indeed, there are already also some other breaking changes already merged and some others in the pipeline |
|
I wonder if it was intended that It seems a strangely different API https://docs.rs/object_store/latest/object_store/trait.ObjectStore.html#tymethod.list_with_delimiter |
Yes it was intended as list_with_delimiter does not return a stream |
|
BTW I think this change has also made it somewhat harder to implement "fail fast" semantics for object store clients, something we saw in IOx and @andygrove noticed while updating ballista slack reference: https://the-asf.slack.com/archives/C01QUFS30TD/p1702312372903249 |
|
The following should do the trick |
|
I ended up with the following to resolve this: stream::once(async { Err(...) }).boxed() |
Which issue does this PR close?
Closes #4946.
Rationale for this change
Having two layers of asynchrony and fallibility makes for a rather obnoxious interface. Removing this makes for better client ergonomics, at the expense of slightly more arcane logic to implement the ObjectStore
What changes are included in this PR?
Are there any user-facing changes?