Conversation
westonpace
left a comment
There was a problem hiding this comment.
This looks pretty cool for opendal. I wonder if we can add support for AIMD (feel free to do this in a future PR). The way non-dal object stores work is that we first retry 3 times (dependent on client_max_retries) and then we trigger the aimd throttle.
I think all we need to do is wrap the object store with AimdThrottledStore and then update the logic on is_throttle_error to recognize opendal temporary errors.
This will add an additional 3 outer retries (configurable via LANCE_AIMD_MAX_RETRIES) for a total of 9 retries. The outer AIMD retries will both:
- Cut the rate in half (it will grow back eventually)
- Have a different delay (on the order of 100-300ms)
Anyways, all of this is fine for future PRs. I think we can merge this once gatekeeper is happy.
| .with_min_delay(Duration::from_millis(100)) | ||
| .with_max_delay(Duration::from_secs(15)) | ||
| .with_factor(2.0) | ||
| .with_jitter(); |
There was a problem hiding this comment.
It would be nice if these were configurable, at least via environment variables. But that can be done in a future PR.
There was a problem hiding this comment.
This looks pretty cool for opendal. I wonder if we can add support for AIMD (feel free to do this in a future PR). The way non-dal object stores work is that we first retry 3 times (dependent on client_max_retries) and then we trigger the aimd throttle.
@westonpace Thanks for reviewing and suggessions! Since @Xuanwo is considering adding AIMD directly in OpenDAL, I’ll leave it out of this PR and follow up upstream in a future change.
It would be nice if these were configurable, at least via environment variables. But that can be done in a future PR.
Agreed. I used fixed values here because the native S3 path also relies on its default backoff configuration and does not expose these settings. The values were chosen to stay close to the S3 defaults without introducing OpenDAL-specific configuration.
There was a problem hiding this comment.
One nuance is that S3 and OpenDAL use different backoff algorithms. S3’s max_backoff caps the final delay after jitter, while OpenDAL’s max_delay caps the base delay before jitter, so 15s can result in an actual delay close to 30s.
Would it make sense to use one shared set of client options for both implementations and accept these small behavioral differences?
client_retry_initial_backoff_ms/OBJECT_STORE_CLIENT_RETRY_INITIAL_BACKOFF_MS, default 100client_retry_max_backoff_ms/OBJECT_STORE_CLIENT_RETRY_MAX_BACKOFF_MS, default 15000client_retry_backoff_factor/OBJECT_STORE_CLIENT_RETRY_BACKOFF_FACTOR, default 2.0
Alternatively, we could expose a separate set of OpenDAL-specific options. @westonpace WDYT?
|
I'm thinking of add AIMD in opendal directly |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
Aside from object storage, do we also need to consider rate limiting and retries on the Namespace? #9113 |
b22a279 to
77fc442
Compare
There was a problem hiding this comment.
The OpenDAL 0.59.2 update resolves the provider-side HTTP 429 classification gap, so throttle responses now reach the shared retry layer. OpenDAL backends still do not enforce Lance’s client_retry_timeout; non-default low-timeout/high-retry configurations can therefore fail later than configured. This is a bounded, visible operational delay rather than a correctness risk, and imposing an outer cancellation timeout could interrupt OpenDAL’s retry state restoration. Until OpenDAL supports an elapsed retry budget, deployments that require a strict deadline should avoid combining a low timeout with a high retry count.
Problem
OpenDAL-backed object stores currently propagate temporary backend errors directly. In distributed writes, transient throttling such as HTTP 429 can fail the entire job even though retrying the request would succeed.
Changes
RetryLayerto S3, Azure Blob/ADLS, GCS, TOS, OSS, COS, Hugging Face, and GooseFS stores.client_max_retries(includingOBJECT_STORE_CLIENT_MAX_RETRIES); the existing default is 3 and 0 disables retries.Testing
cargo fmt --all --checkcargo test -p lance-io --lib --all-features -- --test-threads=1 --skip uring::testscargo clippy -p lance-io --all-features --tests --benches -- -D warnings