perf(storage): remember what a repository holds on the seam and ask once per batch - #133
Conversation
There was a problem hiding this comment.
Traced the cache through usage.rs/backend.rs/objects.rs: Budget is Copy so reusing it across the batch loop is sound, the bucket write path's fresh check (via bucket.exists before staging) correctly prevents double-counting a re-push, and the analogous risk on the adopt path (presigned upload + verify) is already guarded by verify's early exists() return, so a retried verify can't double-adopt. forget_capacity() vs. the seam-level forget(ns) are correctly kept separate (whole-store vs. per-namespace figures). The new re_pushing_an_object_does_not_grow... test is a good, targeted regression check for the trap called out in the description.
Nit: moving buckets onto the same 60s TTL cache the local store already had means bucket quota enforcement goes from "always freshly listed" to "stale up to 60s, shared across all batches/requests to a namespace" — not just within one batch. You call this out in the PR description as an accepted trade-off, but worth double-checking it's also called out in operator-facing docs (quota config), since it's a real loosening of the guarantee specifically for bucket deployments (local deployments already had this laxity).
Nit: Usage::stored increments an existing cache entry even when it's already past TTL (harmless today since the stale timestamp means the next cached() call discards it anyway on the next read), but it's a little subtle — a one-line comment noting that the increment is disposable in the stale case would save the next reader from re-deriving it.
No blockers — logic, tests, and the trade-offs are sound.
9ed0292 to
e3d2255
Compare
SonarQube — aucune nouvelle issueComparaison entre le projet bac à sable de cette PR et la branche par défaut : SonarQube Community n'analyse pas les PR, ce delta est calculé côté CI. Détail |
Closes #125.
resolve_uploadread the repository's usage once per object in the batch, andS3Store::usage_ofanswers that by listing the repository's prefix and issuingone
HEADper marker. So a batch cost objects-asked-for times objects-held roundtrips against a bucket. The local store only escaped it by accident of a cache
that lived inside one backend.
Two changes, both from the issue:
The cache moved to the seam.
Storenow remembers what a repository holds,whichever backend is behind it, so a bucket gets the same policy a volume always
had.
LocalStorekeeps the whole-store figure and exposes an uncachedmeasure_of; the per-namespace memory, the "a stored object adds to it" step andthe "a rewrite drops it" step now live in
storage/usage.rsnext to each other,where a reader can see all three at once. Two implementations of one policy is
one implementation and one that forgot.
The batch asks once. Nothing is written between negotiating and uploading, so
the figure cannot change during the loop in any way that matters. This alone
turns the product back into a sum, and it is what makes the cost independent of
how many objects the client asks about.
Measured
Same shape as the issue: a batch of 20 against a repository holding 40 objects,
MinIO on localhost.
The second batch is the point: within the cache window a quota check is no longer
distinguishable from having no quota at all. And the remaining 72.9 ms is now one
measurement per batch instead of one per object, so it stops growing with the
batch size.
A correctness trap this opened, and the test for it
Remembering a figure means adding to it, and the "is this object new" flag came
from the staging store. On a bucket deployment that store's object layout is
never filled in, so it called every upload new: re-pushing an object the
repository already held would have grown what it was said to hold without
anything being stored, and eventually refused space that was never used. It is
now asked of the bucket, which costs one
HEADper upload against a transferthat is already streaming the bytes.
re_pushing_an_object_does_not_grow_what_the_repository_is_said_to_holdcoversit. Checked against the broken version first: it reports 614400 where the
repository holds 307200, which is the drift, doubled on one repeat.
Verification
cargo fmt --check,cargo clippy --all-targets -- -D warningsread once instead of per object