Add bulk execution-result persistence - #47
Open
BinaryFiddler wants to merge 4 commits into
Open
BinaryFiddler wants to merge 4 commits into
BinaryFiddler wants to merge 4 commits into
Conversation
BinaryFiddler
force-pushed
the
chenyu/stored-result-bulk-write
branch
2 times, most recently
from
August 20, 2026 19:15
cbb5089 to
6ee287e
Compare
Stored execution results are written one row per action: at ~6.2k actions/s in
production that is ~6.2k Bigtable MutateRows calls/s, one per action. This adds
the ordered bulk contract a batching caller needs to collapse those into one
call per batch. Bigtable behaviour on the existing single-row path is unchanged
apart from the new metrics; the retry fix is the following commit.
- ExecutionResultStore.insert_many(): one outcome per write, aligned to input
order, with a one-by-one fallback for backends that have no bulk path.
StoredExecutionResultBigTable overrides it with a single MutateRows call per
chunk of at most 25,000 rows (Bigtable's 100,000-mutation cap at four
cells/row) and surfaces each returned row status independently.
- One row-format helper builds both the single and the bulk row, so the two
paths cannot drift, and insert() stays self-contained so removing the bulk
override falls back to one-by-one instead of recursing.
- Staged failure semantics: the bulk path returns typed error outcomes, while
insert() counts a failed row status in row_outcome{outcome:row_error} without
raising. The pre-bulk contract discarded these statuses, and raising costs
three sink attempts plus a Sentry event per row at an unmeasured rate.
- Encoded size is measured before mutate_rows(), not after: the client clears
the mutations of every row it commits, so bytes read afterwards report only
the row keys.
- commit_ms and row_outcome carry path:single|bulk so the two write paths stay
separable while both are live. row_outcome aggregates per distinct outcome,
so a 25,000-row chunk failing one way is one datagram rather than 25,000.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Table.mutate_rows signals retryable work only by raising _BigtableRetryableError. The read-path retry_policy's transient-error predicate never matches that, so no client-side mutation retry has ever run on this path — a retryable row status returned immediately and was discarded. Mutations now use a DEFAULT_RETRY-derived policy, but bounded to a 2s deadline rather than DEFAULT_RETRY's 120s. 120s outlasts every caller of this store: the async stored-result sink abandons a write after 5s (its sync counterpart after 2s) and runs it in asyncio.to_thread(), so abandoning it cannot cancel the thread. A 120s in-call retry would hold a pool slot for a result nobody reads, and a broad Bigtable brownout would park every in-flight write there at once. That sink already times out more than any other by two orders of magnitude, so the path is well exercised. Longer recovery stays the sinks' job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
BinaryFiddler
force-pushed
the
chenyu/stored-result-bulk-write
branch
from
August 20, 2026 19:28
6ee287e to
6300cc3
Compare
_encoded_request_size() ran outside the chunk's try, so a failure to measure raised out of insert_many() before mutate_rows() was called: the batch was never written, and the caller got an exception instead of the one-outcome-per-write list the contract promises. A row-build failure is already contained as batch_error outcomes; measurement — pure instrumentation — was not. Building a MutateRowsRequest to measure has more ways to fail than the arithmetic it replaced (proto field validation, table attribute access), so guard it: log, drop the batch_bytes sample, and let the write proceed. batch_rows still reports the completed call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds ordered bulk persistence for stored execution results, enabling the Smite coordinator to batch Bigtable writes. This PR only adds sink support; batching begins when a caller opts in.
Changes
insert_many()/persist_writes(), returning one ordered outcome per input and falling back toinsert()for non-bulk backends.Row encoding, idempotency, staged failure behavior, reads, and non-Bigtable backends are unchanged.
Rollback
Remove the Bigtable
insert_many()override to restore the one-by-one fallback.Verification