Make the GCS simple-upload threshold configurable - #2729
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
MarcusSorealheis
left a comment
There was a problem hiding this comment.
The bounded single-request path is a useful optimization. Before merging, please complete the user-facing documentation and add coverage for the compression composition and failure paths below. This review is based on source inspection and the existing CI results; I have not run new benchmarks.
For expansion, I would keep this PR focused on GCS and follow with shared bounded-buffering logic for S3 (which also covers the R2 and OCI adapters), ONTAP S3, and Azure. Preserve backend-specific checksums, retries, and upload limits. MongoDB already buffers before an upsert, so it does not have the same multipart request-count opportunity.
| /// Uploads whose exact size is unknown but whose declared upper bound is | ||
| /// below this threshold are buffered in memory and written with one | ||
| /// single-request upload instead of a resumable session (one `POST` to | ||
| /// open the session, one `PUT` per chunk and one `GET` to verify). | ||
| /// Values above 5MB are clamped to 5MB, the same bound that uploads of | ||
| /// known exact size already use for the single-request path. | ||
| /// When unset, uploads of unknown exact size always use a resumable | ||
| /// session, regardless of their declared upper bound. | ||
| /// | ||
| /// Default: unset (unknown-size uploads always use a resumable session) |
There was a problem hiding this comment.
Please make the configuration semantics explicit in this source-of-truth comment, then regenerate the config reference with gen:config-reference rather than editing generated pages. Suggested replacement:
| /// Uploads whose exact size is unknown but whose declared upper bound is | |
| /// below this threshold are buffered in memory and written with one | |
| /// single-request upload instead of a resumable session (one `POST` to | |
| /// open the session, one `PUT` per chunk and one `GET` to verify). | |
| /// Values above 5MB are clamped to 5MB, the same bound that uploads of | |
| /// known exact size already use for the single-request path. | |
| /// When unset, uploads of unknown exact size always use a resumable | |
| /// session, regardless of their declared upper bound. | |
| /// | |
| /// Default: unset (unknown-size uploads always use a resumable session) | |
| /// Uploads with an unknown exact size use a single-request upload when | |
| /// their declared upper bound is strictly below this threshold. The store | |
| /// buffers the stream through EOF and uploads its actual length. A stream | |
| /// exceeding its declared bound is rejected before an upload request. | |
| /// | |
| /// The effective threshold is capped at 5 MiB (5,242,880 bytes). A bound | |
| /// equal to the threshold still uses a resumable upload. Exact-size | |
| /// uploads retain their existing behavior regardless of this setting. | |
| /// | |
| /// This threshold applies to the declared bound, not the final object | |
| /// size. Compression can produce a bound larger than the stored object. | |
| /// Buffering occurs before acquiring a connection permit; this setting | |
| /// does not bound aggregate upload memory or account for request copies. | |
| /// | |
| /// Default: unset. Unset or zero leaves unknown-size uploads resumable. |
Also update web/apps/docs/content/docs/how-to/stores/gcs.mdx, under Options worth setting. Its current “everything else uses a resumable upload” statement becomes incomplete once this option is enabled. Add a table row and a short opt-in example inside the existing GCS provider config, such as simple_upload_threshold: 5242880 (5 MiB), then explain:
With
simple_upload_thresholdset, an upload whose exact size is unknown can also use one request when its declared maximum is strictly below the effective threshold. Compression supplies a conservative maximum, so choose the threshold using that bound rather than the final compressed size. With the default 64 KiB compression blocks, even a tiny object has a declared maximum greater than 64 KiB. Uploads buffer before waiting for a connection, so monitor memory as well as request counts when enabling this option.
Link the generated field reference for the remaining details, retain the page's explicit anchors/source links, and run the docs snippet and anchor checks. The new field and backend guide are required follow-ups under AGENTS.md.
|
|
||
| /// Drives `update()` with `upload_size`, sending `data` in `chunk_size` pieces | ||
| /// so the store cannot infer the total length from the first chunk. | ||
| async fn run_update_in_chunks( |
There was a problem hiding this comment.
Please add a test of the motivating CompressionStore -> GcsStore composition. These direct MaxSize(300) tests exercise the routing well, but compression's UploadState::new calculates its bound using whole blocks: with the default 64 KiB block, a tiny input's bound is already greater than 64 KiB. A regression test should use the real bound, assert that a threshold below it stays resumable, and that a threshold above it produces one write and round-trips through decompression.
Please also cover the newly enabled path with a transient write failure followed by successful replay of identical bytes, sender termination without EOF (no upload request), and overflow delivered in one chunk larger than the bound. Empty MaxSize streams, including MaxSize(0) with a non-zero-digest key, would round out the boundary cases. Use a producer/consumer harness that preserves the store error when the producer sees the reader close early.
| _ => None, | ||
| }; | ||
| if let Some(bound) = simple_upload_bound { | ||
| let content = reader.consume(Some(usize::try_from(bound)?)).await?; |
There was a problem hiding this comment.
Please qualify the PR description's claim that memory per in-flight upload is bounded by 5 MB. The accepted payload is capped below 5 MiB, but content.to_vec() allocates another full copy before GcsClient::write_object acquires its connection permit; the reader can also retain the original chunks in its retry buffer. Consequently, queued uploads consume memory too, and the connection count is not a bound on the number of buffered uploads.
The existing exact-size path already has this copy, so this is not a newly introduced allocation bug. However, this PR brings compressed MaxSize traffic onto that path. Please document the distinction and avoid claiming a hard 5 MiB memory ceiling. Before making this the default or extending it broadly, measure RSS with concurrent uploads waiting for permits; a replayable Bytes body and a separate buffering budget are possible follow-ups.
## What and why `GcsStore::update` only takes the single-request simple-upload path when the caller passes `UploadSizeInfo::ExactSize` below 5MB. Every upload that arrives as `UploadSizeInfo::MaxSize` goes through a resumable session: `start_resumable_write` (POST), one `upload_chunk` (PUT) and an `object_exists` (GET) verification, each acquiring the shared connection permit in turn. `CompressionStore` can only ever pass `MaxSize`, because the compressed length is not known until the stream has been consumed, so behind a compression store a 300-byte action result costs three GCS requests and three trips through the permit queue. This adds `simple_upload_threshold` to `ExperimentalGcsSpec`, a sibling of `resumable_chunk_size`. When set, a `MaxSize(max)` upload with `max` strictly below the threshold is buffered through EOF (at most `max` bytes) and written with one `write_object` through the existing retrier. A stream that keeps producing bytes past its declared bound is rejected with `InvalidArgument` before any request is issued. The threshold is capped at `MIN_MULTIPART_SIZE` (5 MiB), so a bounded unknown-size upload is never treated more aggressively than an exact-size one. The threshold applies to the declared bound, not the final object size. `CompressionStore` computes its bound in whole blocks, so with the default 64 KiB block even a tiny input declares more than 64 KiB; the threshold has to be chosen against that bound. Unset is the default and keeps the current behavior exactly: `MaxSize` always resumable, `ExactSize` below 5MB simple as before, zero-digest fast path untouched. The GCS how-to page documents the option under "Options worth setting" with an opt-in example. `gen:config-reference` was run and produced no diff: the generated reference renders `ExperimentalCloudObjectSpec` as a stub, so provider fields only surface through the schema. The field is allowlisted in `lint-snippets.mjs` next to `sas_url`, which exists for the same reason. Measured on a NativeLink v1.6.3 deployment (GCS slow tier behind a compression store, Redis fast tier) under a warm-cache lookup storm: the resumable round trips for small writes helped back up the per-pod permit FIFO past the CAS server's 30s per-blob deadline. Together with two other changes, enabling this took GetActionResult p99 from 30.0s to under 1.2s and cut GCS request volume by roughly 6x. ## How was this verified? Fifteen tests added to `nativelink-store/tests/gcs_store_test.rs`. The direct tests drive `update()` through a producer/consumer harness that runs both halves concurrently and returns the store's error ahead of the producer's, so a reader closed early cannot mask the cause. The composition tests wrap the mock-backed `GcsStore` in a real `CompressionStore` with the default 64 KiB block, so the declared bound is the one compression actually computes: - default config, `MaxSize(300)`: zero `write_object`, one `start_resumable_write`, one `upload_chunk`, one `object_exists` - threshold set, `MaxSize(300)`: exactly one `write_object`, zero resumable calls, content round-trips through `get_part` - threshold set, stream shorter than the bound: one `write_object` of the actual length - threshold set, stream longer than the bound in 1-byte chunks: `InvalidArgument` and zero requests recorded by the mock - threshold set, overflow delivered as one chunk larger than the bound: `InvalidArgument` and zero requests - bound equal to the threshold: resumable (strict less-than) - threshold set, `MaxSize(6MiB)`: resumable with several chunks - threshold configured above 5MB, `MaxSize(6MiB)`: still resumable (cap) - compression over GCS, threshold 64 KiB: a 300-byte input stays resumable, because its declared bound exceeds one block - compression over GCS, threshold 5 MiB: exactly one `write_object` and the content round-trips through decompression - transient `Unavailable` on the first `write_object`: the retry replays identical bytes, only the replay reaches the backend - sender dropped without EOF: the store's `Internal` error surfaces and no request is issued - empty stream with `MaxSize(300)`: one empty `write_object`, `has` reports size 0 - empty stream with `MaxSize(0)` and a non-zero-digest key: one empty `write_object`, no resumable session - one byte on a `MaxSize(0)` stream: `InvalidArgument`, zero requests With the `gcs_store.rs` change reverted and the tests kept, seven tests fail (the five threshold-on routing tests, the compression single-write test and the transient-failure test) and the other eight pass. The `build-schema` binary of `nativelink-config` (feature `dev-schema`) emits the new field as an optional `uint64`. `lint:anchors`, `lint:snippets` and `lint:links` pass on the docs change. ## Risk Default unset means no behavior change for existing configurations. When enabled, the only new failure mode is a stream that violates its declared `MaxSize` bound, which now fails with `InvalidArgument` before contacting GCS instead of being accepted by the resumable path; no in-tree caller produces such a stream. Memory is not hard-capped at 5 MiB per upload. The accepted payload is capped below 5 MiB, but `content.to_vec()` makes a second full copy before `write_object` acquires its connection permit, and the reader can retain the original chunks in its retry buffer. Uploads waiting for a permit therefore hold memory too, and the connection count does not bound the number of buffered uploads. This copy already exists on the exact-size simple path; what this change does is route compressed `MaxSize` traffic onto it. Measure RSS with concurrent uploads waiting for permits before enabling it broadly or making it the default. `ExactSize` uploads and the zero-digest path are untouched.
777d206 to
cace162
Compare
|
Pushed a revision against all three comments. Doc comment: your text verbatim. Tests: Memory: the Risk section now describes the second copy and permit-queue buffering, no ceiling claimed. Agreed on GCS-only here; S3, ONTAP and Azure as a follow-up. |
What and why
GcsStore::updateonly takes the single-request simple-upload pathwhen the caller passes
UploadSizeInfo::ExactSizebelow 5MB. Everyupload that arrives as
UploadSizeInfo::MaxSizegoes through aresumable session:
start_resumable_write(POST), oneupload_chunk(PUT) and an
object_exists(GET) verification, each acquiring theshared connection permit in turn.
CompressionStorecan only ever passMaxSize, because the compressed length is not known until the streamhas been consumed, so behind a compression store a 300-byte action
result costs three GCS requests and three trips through the permit
queue.
This adds
simple_upload_thresholdtoExperimentalGcsSpec, a siblingof
resumable_chunk_size. When set, aMaxSize(max)upload withmaxstrictly below the threshold is buffered through EOF (at most
maxbytes) and written with one
write_objectthrough the existingretrier. A stream that keeps producing bytes past its declared bound is
rejected with
InvalidArgumentbefore any request is issued. Thethreshold is capped at
MIN_MULTIPART_SIZE(5 MiB), so a boundedunknown-size upload is never treated more aggressively than an
exact-size one.
The threshold applies to the declared bound, not the final object size.
CompressionStorecomputes its bound in whole blocks, so with thedefault 64 KiB block even a tiny input declares more than 64 KiB; the
threshold has to be chosen against that bound.
Unset is the default and keeps the current behavior exactly:
MaxSizealways resumable,
ExactSizebelow 5MB simple as before, zero-digestfast path untouched.
The GCS how-to page documents the option under "Options worth setting"
with an opt-in example.
gen:config-referencewas run and produced nodiff: the generated reference renders
ExperimentalCloudObjectSpecasa stub, so provider fields only surface through the schema. The field
is allowlisted in
lint-snippets.mjsnext tosas_url, which existsfor the same reason.
Measured on a NativeLink v1.6.3 deployment (GCS slow tier behind a
compression store, Redis fast tier) under a warm-cache lookup storm:
the resumable round trips for small writes helped back up the per-pod
permit FIFO past the CAS server's 30s per-blob deadline. Together with
two other changes, enabling this took GetActionResult p99 from 30.0s
to under 1.2s and cut GCS request volume by roughly 6x.
How was this verified?
Fifteen tests added to
nativelink-store/tests/gcs_store_test.rs. Thedirect tests drive
update()through a producer/consumer harness thatruns both halves concurrently and returns the store's error ahead of
the producer's, so a reader closed early cannot mask the cause. The
composition tests wrap the mock-backed
GcsStorein a realCompressionStorewith the default 64 KiB block, so the declared boundis the one compression actually computes:
MaxSize(300): zerowrite_object, onestart_resumable_write, oneupload_chunk, oneobject_existsMaxSize(300): exactly onewrite_object, zeroresumable calls, content round-trips through
get_partwrite_objectofthe actual length
InvalidArgumentand zero requests recorded by the mockbound:
InvalidArgumentand zero requestsMaxSize(6MiB): resumable with several chunksMaxSize(6MiB): still resumable(cap)
resumable, because its declared bound exceeds one block
write_objectand the content round-trips through decompression
Unavailableon the firstwrite_object: the retryreplays identical bytes, only the replay reaches the backend
Internalerror surfacesand no request is issued
MaxSize(300): one emptywrite_object,hasreports size 0
MaxSize(0)and a non-zero-digest key: one emptywrite_object, no resumable sessionMaxSize(0)stream:InvalidArgument, zero requestsWith the
gcs_store.rschange reverted and the tests kept, seven testsfail (the five threshold-on routing tests, the compression single-write
test and the transient-failure test) and the other eight pass. The
build-schemabinary ofnativelink-config(featuredev-schema)emits the new field as an optional
uint64.lint:anchors,lint:snippetsandlint:linkspass on the docs change.Risk
Default unset means no behavior change for existing configurations.
When enabled, the only new failure mode is a stream that violates its
declared
MaxSizebound, which now fails withInvalidArgumentbeforecontacting GCS instead of being accepted by the resumable path; no
in-tree caller produces such a stream.
Memory is not hard-capped at 5 MiB per upload. The accepted payload is
capped below 5 MiB, but
content.to_vec()makes a second full copybefore
write_objectacquires its connection permit, and the readercan retain the original chunks in its retry buffer. Uploads waiting for
a permit therefore hold memory too, and the connection count does not
bound the number of buffered uploads. This copy already exists on the
exact-size simple path; what this change does is route compressed
MaxSizetraffic onto it. Measure RSS with concurrent uploads waitingfor permits before enabling it broadly or making it the default.
ExactSizeuploads and the zero-digest path are untouched.This change is