enhance: make Lance AIMD IOPS limits configurable

[jiaqizho]

Lance already throttles ObjectStore requests with AIMD, but milvus-storage had no filesystem-level way to control the starting rate or ceiling. Callers had to rely on Lance defaults or process-wide environment variables, which is awkward when one process serves multiple storage domains with different throughput requirements.

This change adds fs.iops_initial_rate and fs.iops_max_rate as Lance-only filesystem properties. Their defaults match Lance at 2,000 and 5,000 requests per second. The initial rate must be positive, while a maximum rate of zero removes the ceiling. Bounded configurations also reject an initial rate that exceeds the maximum.

Since these rates change ObjectStore behavior, they participate in the I/O-domain fingerprint. Datasets using different AIMD configurations will not accidentally share the same throttled ObjectStore or scheduler domain, while scheduler parallelism remains outside the identity key.

[sre-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jiaqizho
To complete the pull request process, please assign tedxu after the PR has been reviewed.
You can assign the PR to them by writing /assign @tedxu in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[jiaqizho]

base on 615

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.44%. Comparing base (a6fb325) to head (54ba65a).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #618      +/-   ##
==========================================
+ Coverage   76.42%   76.44%   +0.01%     
==========================================
  Files         173      173              
  Lines       17564    17571       +7     
  Branches     2656     2657       +1     
==========================================
+ Hits        13424    13432       +8     
+ Misses       4140     4139       -1     

Flag	Coverage Δ
cpp	79.03% <100.00%> (+0.01%)	⬆️
python	44.45% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[buqian-zilliz]

[P1] Avoid multiplying GCS retries under the AIMD wrapper

The impersonation client above is still built with RetryConfig::default() (10 retries and a 180-second timeout), and this new wrapper adds up to three more AIMD retries around each exhausted inner operation. A persistent 429/503 can therefore issue 44 GCS attempts and consume four separate retry windows before failing; it also ignores Lance's client_max_retries setting, including zero. Please construct the inner retry config from Lance StorageOptions and apply the same zero-retry guard as the stock GCS provider before installing AIMD.

[jiaqizho]

ack

[buqian-zilliz]

[P1] Keep canceled reads from poisoning the shared scheduler

Lance's decode stream aborts its scheduling task on early stream drop so that the operation's ScanScheduler can be dropped and pending I/O canceled. Reusing the scheduler here means another live dataset keeps that Arc alive; if the dropped stream also drops a request's oneshot receiver, task completion returns the IOP slot but never runs on_bytes_consumed, leaving buffered-byte and priority accounting charged. A caller that releases an Arrow stream before EOF can therefore backpressure unrelated datasets until the entire domain becomes idle. Please add per-request cleanup/cancellation or share a lower-level permit limiter instead of the scheduler object.

[jiaqizho]

ack

[buqian-zilliz]

[P2] Preserve per-operation priority fairness

ScanScheduler is a priority queue, not just a concurrency semaphore. Every fragment reader installed by this bridge uses FragReadConfig::default(), so its reader priority stays 0 and Lance orders the remaining work by file-local position. Once one queue is reused across independent datasets, unrelated offsets compete in the same heap; sustained low-position reads from one dataset can continuously outrank higher-position work from another. Please keep a per-operation priority domain, or share only the concurrency permits beneath separate schedulers.

[buqian-zilliz]

This issue still exists: shared fragment readers are still created from FragReadConfig::default() without a per-operation reader_priority, so unrelated datasets continue to compete in one priority heap using file-local positions.

[liliu-z]

Because fs.iops_initial_rate defaults to 2000, setting only fs.iops_max_rate to anything in [1, 1999] — the natural way to lower the ceiling — fails this check and aborts create_file_system_config. That function is the shared config path (resolve_config at fs.cpp:410 feeds lance_format, iceberg_format, paimon_format and FilesystemCache::get), so a Lance-only knob takes down filesystem creation for Parquet/Iceberg/Paimon too. Consider clamping the initial rate down to the configured max instead of erroring, or applying the check only where Lance actually consumes the values.

[jiaqizho]

it's fine, this config is global not just for lance

[liliu-z]

lance_common.cpp:33-34 writes AIMD keys into the storage options unconditionally rather than only when the corresponding property was explicitly set, so absent properties silently inject values that collide with the environment-variable path. When a user sets min_rate via env but initial_rate arrives from the always-populated storage options, the mix is inconsistent and AimdConfig::validate() hard-fails — an outright error, not a silent override, so no amount of documenting the precedence prevents it. Track explicit property presence and omit absent keys from the storage options map.

[jiaqizho]

expected. Should never use env to overwrite the properties

[weiliu1031]

[P1] Preserve whether the AIMD properties were explicitly configured

These storage options are emitted unconditionally, so even a caller that never sets fs.iops_initial_rate or fs.iops_max_rate sends the defaults 2000 and 5000 to Lance. Lance resolves storage options before environment variables, so this silently changes the behavior of existing deployments that configure LANCE_AIMD_INITIAL_RATE or LANCE_AIMD_MAX_RATE.

It can also produce an invalid mixed configuration because the remaining AIMD settings are not exposed by milvus-storage and still come from the environment. For example, LANCE_AIMD_MIN_RATE=3000 combined with the injected default initial rate of 2000 makes AimdConfig::validate() reject every ObjectStore open, even though the caller did not configure either new property.

Please retain explicit-property presence in ArrowFileSystemConfig and emit these keys only when the caller actually supplied them. Alternatively, expose and validate the complete AIMD configuration and consistently disable the environment fallback instead of combining property defaults with unrelated environment values.

[liliu-z]

Before this change the non-role Aliyun path fell through to _ => {} and used Lance's stock OSS provider, which applies no AIMD throttle at all; now every OSS read is paced by a token bucket starting at 2000 req/s and ramping to 5000. The PR description's claim that the defaults "match Lance" holds for S3/Azure/GCS but not for OSS, where this is a brand-new ceiling for deployments that never opted in. It is also one-directional: the provider's own doc comment notes OSS 429/503 responses are not recognized by Lance's throttle-error detector, so the controller can only increase, making this a static cap rather than adaptive throttling. Either default OSS to the disabled throttle or call the change out in the release notes.

[jiaqizho]

expect, do need iops limit for OSS

[weiliu1031]

[P1] Do not rely on Lance's error-string heuristic for OSS AIMD feedback

AimdThrottledStore recognizes a throttled request only when it receives an object_store::Error::Generic whose source text contains "retries, max_retries". Both Aliyun providers here are backed by OpenDAL, and their OSS 429/503 responses do not produce that native object_store retry-error format.

Consequently, real OSS throttling is classified as success: the controller does not perform multiplicative decrease, the AIMD retry loop is not entered, and the rate may continue increasing by the additive increment. Conversely, the string heuristic cannot distinguish a throttle response from another exhausted-retry error carrying the same text. The meaning of these properties therefore becomes provider-dependent, and on OSS this wrapper is effectively a proactive token bucket rather than adaptive AIMD.

Please add a typed OSS throttle classifier that maps the OpenDAL HTTP status or raw response into an explicit throttle signal before enabling adaptive behavior. If that is not currently possible, keep OSS throttling opt-in and document it as a static token-bucket limit. A fault-injection test using real 429 and 503 responses should verify both rate reduction and retry behavior.

[liliu-z]

CloudConfiguredAimdRateLimit asserts a rate-plus-slack ceiling but leaves LANCE_AIMD_BURST_CAPACITY unpinned, so an environment that raises the burst above the ~250 slack makes the test fail for reasons unrelated to the code — the sibling Aliyun test added in this same PR pins that variable explicitly. The measured quantity is also broader than the assertion implies: peak_one_second_iops accumulates read_iops, which Lance increments for list* as well as get*, while the throttle budgets read and list in separate buckets. Pin the burst capacity in this test and assert against a read-only counter.

[jiaqizho]

ack

[buqian-zilliz]

[P1] Do not turn stream cancellation into a full background read

stream is not limited to already-issued work. For large reads Lance attaches a background scheduling task to the decode stream and aborts that task in the stream's on_drop handler. Polling to EOF here prevents that cancellation path, so releasing an Arrow stream after a LIMIT, error, or request cancellation continues scheduling and decoding the entire remaining range in a detached task. Large abandoned reads can therefore keep consuming object-store bandwidth, CPU, and the shared domain's scheduler capacity long after the caller is gone, which can cascade into latency for live readers. Please make the per-request reservation cleanup cancellation-safe (or share a lower-level limiter) instead of draining every canceled stream to completion.

[liliu-z]

Both Aliyun providers now install AimdThrottledStore unless the config reports itself disabled, and Lance defines disabled as max_retries == 0 — a key milvus-storage never emits, so the wrapper is unconditionally on. Stock lance-io has no throttle on its OSS provider, so this newly paces all OSS reads and writes through a token bucket capped at 5000 rps by default; fs.iops_max_rate=0 only lifts the ceiling, it does not remove the wrapper, and the only real off switch is the process-wide LANCE_AIMD_MAX_RETRIES env var this PR is otherwise trying to avoid. Worth either making the OSS wrapper opt-in or surfacing a property that maps to lance_aimd_max_retries=0, especially since your own doc comment notes the adaptive half is inert on these OpenDAL-backed stores.

[weiliu1031]

[P1] Do not describe a per-store, per-category rate as a filesystem ceiling

fs.iops_max_rate appears to provide one aggregate request-rate ceiling for the filesystem or I/O domain, but Lance creates four independent token buckets (read, write, delete, and list) for every AimdThrottledStore. In addition, streaming list, list_with_offset, and delete_stream operations do not acquire tokens, and different ObjectStore instances have independent limiter state. Only the fragment-read paths that reuse the same shared scheduler are guaranteed to share one throttled ObjectStore.

As a result, concurrent operation categories or multiple active ObjectStore instances can exceed the configured value, potentially by a multiple of fs.iops_max_rate. This is especially risky if callers use this setting to stay below a bucket/account quota.

If the intended contract is an aggregate filesystem or I/O-domain limit, please move the token budget to the shared I/O-domain layer. Otherwise, rename and document this property explicitly as a per-ObjectStore, per-operation-category rate, and add a test demonstrating the expected aggregate behavior.

[weiliu1031]

/lgtm