adriangb opened a new pull request, #808:
URL: https://github.com/apache/arrow-rs-object-store/pull/808
# Which issue does this PR close?
No separate issue is filed; the bug and repro are described in full below.
# Rationale for this change
`ThrottledStore` is the crate's built-in wrapper for performance testing (it
injects `wait_get_per_call` first-byte latency, `wait_get_per_byte` throughput
throttling, and so on). But it panics with `not implemented` on the first
`get`/`get_opts`/`head` when it wraps a store that returns a file-backed
payload, which is exactly what `LocalFileSystem` does when the `fs` feature is
enabled, and `LocalFileSystem` is the obvious inner store you would want to
throttle for perf testing.
Every `get_opts` result is funnelled through the free function
`throttle_get` in `src/throttle.rs`, whose payload match was:
```rust
GetResultPayload::File(_, _) => unimplemented!(),
```
`LocalFileSystem::get_opts` returns `GetResultPayload::File(..)`, so the
first read hits `unimplemented!()`.
Note that the fixed per-call latency already worked for File payloads
(`sleep(wait_get_per_call)` runs before the inner call in `get_opts`), and
`get_ranges` already worked (it delegates directly). Only the per-byte
stream-wrapping step in `throttle_get` panicked.
Minimal repro:
```rust
use object_store::{throttle::{ThrottledStore, ThrottleConfig},
local::LocalFileSystem, ObjectStore, path::Path};
let dir = tempfile::TempDir::new().unwrap();
let store = ThrottledStore::new(
LocalFileSystem::new_with_prefix(dir.path()).unwrap(),
ThrottleConfig::default(),
);
let path = Path::from("file.txt");
store.put(&path, "hello".into()).await.unwrap();
store.get(&path).await.unwrap(); // panics: not implemented
```
# What changes are included in this PR?
`throttle_get` now handles the File payload instead of panicking. The design
preserves the local-file fast path rather than forcing every File read into a
byte stream just so it can be throttled:
- For a `Stream` payload, behavior is byte-for-byte unchanged: the per-byte
throttle is applied lazily per chunk as before.
- For a `File` payload, the per-byte component is applied as a single eager
sleep computed from the known requested range (`result.range`), mirroring how
`get_ranges` already throttles a whole vectored read up front, and then the
File payload is returned untouched. Because `sleep` is a no-op on a zero
duration, the common case where only `wait_get_per_call` is set (and
`wait_get_per_byte` is zero) passes the File payload straight through with no
allocation or stream conversion.
`throttle_get` becomes `async` so the File-path sleep can be awaited; the
single caller in `get_opts` awaits it.
A regression test (`get_local_file_system`) wraps a real `LocalFileSystem`
in a `ThrottledStore` with non-zero per-call and per-byte waits, reads an
object back, asserts the payload is still a `File` (fast path preserved), and
asserts the bytes are correct. This test panics on the previous code and passes
now.
# Are there any user-facing changes?
Yes, a bug fix: `ThrottledStore` no longer panics when wrapping a store that
returns a `GetResultPayload::File` (e.g. `LocalFileSystem`). No public API
changes; `throttle_get` is a private function.
--
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]
