client: a dozen commands in one round trip, with an answer apiece (#11) - #44
Merged
Conversation
`BatchRequest` and `Client::execute_batch` -- the cluster's `execute_batch`, which both official clients have had all along (#11). A launcher that creates a dozen tables no longer makes a dozen round trips, and the answer is a Vec of per-part Results, because that is what a batch is: its parts fail individually, and collapsing them into one Result would lose the only thing batching costs any clarity on. Each Ok carries the part's own v4 envelope, keyed by what that command returns (`{node_id=...}` for a create, `{value=...}` for an exists); each Err is an ordinary ClientError::Cluster named after the part's command, flattened outer-plus-innermost like every other. The building shape is a builder rather than a slice of prepared commands: typed methods (create, create_table, exists, get, list, remove, remove_tree, set_attribute) that send exactly what their Client namesakes send, plus a raw escape hatch. A builder is what lets the retry class be decided instead of guessed: parts this crate models are Cypress commands the master's mutation cache covers, so a mutating batch retries under a mutation id -- the driver hands part k the batch's id plus k (GenerateNextBatchMutationId) and stamps the batch's retry flag into every volatile part, so the master answers a marked replay with each part's first response. Measured on a local cluster with parts that carry no ignore_existing -- create_table, not create: a two-part batch replayed under its id with retry=%true answered the same two node ids both times, where a fresh id got two 501s. The same probe with BatchRequest::create proves nothing, because ignore_existing answers with the old node's id under any id at all. A batch of nothing but reads carries no id at all, and a batch holding a raw part is sent once, whatever the policy says, because a command this crate cannot classify may mutate somewhere no mutation cache covers. Both official options travel: with_concurrency, the command's own server-side cap (default 50 in the cluster's registration), and with_max_part_size, the C++ client's BatchPartMaxSize -- a bigger batch is split into consecutive requests, concurrency x 5 apiece unless told otherwise, results stitched back in part order and a mutation id per request. The C++ client also re-queues a retriable part client-side; this one deliberately does not, and docs/sdk-comparison.md discloses it. Two wire decisions with measurements behind them. The batch's parameters go in the POST body, where every other command's ride in X-YT-Parameters: a batch's parameters are the batched commands, a header has a size nobody promises, the C++ client makes the same choice for this same command, and the proxy merges body parameters with the header's (TContext::CaptureParameters) -- requests in the body and mutation_id in the header measured landing as one parameter set. And a client bound to a transaction stamps the parts, not the envelope: execute_batch has no transactional options, and a local cluster dropped an outer transaction_id in silence -- the part's create landed outside the transaction and survived its abort, which is the silent escape a transaction exists to prevent. execute_batch therefore joins NO_TRANSACTION, and each part is stamped with the transport's own exceptions. The parser refuses what it does not recognise -- a results count that does not match the parts, an item that is neither {output=...} nor {error=...} nor the empty map a null-output command answers with -- rather than reading it as somebody's success. Verified live end to end by `cargo run -p ytsaurus-client --example batch`: twelve creates in one round trip and every node there afterwards, one part failing (create over an existing node) with the rest succeeding in order, a split batch keeping a failure at its own index, and a batch inside a transaction invisible until the commit. Also watched: parts run in parallel (an exists beside its own create answered %false), and a part naming an unknown command fails the whole batch with no per-part results. tests/batch.rs pins the wire shape against an in-process stub that reads the whole request before answering -- verb, body, the mutation id repeated across a retry and absent from a read-only batch -- decoding X-YT-Parameters and comparing values, never asserting on the rendered text of a generated id.
…ed for, a stricter parser, a wider raw door
The feature works; these are the honesty gaps two reviews found in it.
A split batch that stopped mid-sequence used to return the failure and
nothing else, with the earlier requests already applied and no way for the
caller to learn it — and re-running the same BatchRequest is not a recovery,
because a second execution mints fresh mutation ids. `ClientError::BatchInterrupted`
now carries the prefix that was answered for, beside the count and the cause.
A batch that fits one request is unchanged. Measured on a local cluster while
writing the test for it: a request refused wholesale can still have applied
some of its parts — a create beside a part naming an unknown command created
its node — so the error reports what came *back* and does not claim to know
what landed.
The bare `{}` success arm accepted "no output" for every command, including
the ones whose success has a value in it. A cluster answering `{}` to a
create passed the parser and then panicked in caller code at
`answer["node_id"]`, which is the access the rustdoc, the example and the
tests all teach. The arm is now restricted to the commands the driver source
names — the null-output ones — plus raw, whose registry bits only its caller
knows.
The raw door in a batch was narrower than `Client::raw_command_with`:
`BatchRequest::raw_with` takes the Repeatable, so one raw *read* no longer
demotes an all-read batch to send-once, and `Client::execute_batch_with`
takes the MutationId, so the crash-replay guarantee is expressible through
the API rather than only around it. Reproduced through it on a local cluster,
with create_table parts because they carry no ignore_existing: the same two
node ids under a replayed id, two 501s under a fresh one. Run the same probe
with BatchRequest::create and it proves nothing -- ignore_existing answers with
the old node's id whether or not a replay was recognised.
A command the cluster will not take as a part is refused where it is written
rather than costing a round trip.
Documented what was undocumented: the redirect a batch body cannot follow
where the same creates sent individually can, the consuming option setters
beside the borrowing part adders, and what executing one batch twice does.
AGENTS.md gains the cluster facts — the silently dropped envelope
transaction, the whole-batch failure on an unknown part command and that it
is not a rollback, the parallel parts, the per-part replay — and the example
stops claiming a round-trip count it cannot see: tests/batch.rs counts it.
…to its key
A second review found four cluster facts written down wrong. All four are
re-measured here against a local v4 cluster, and the code moved wherever a
measurement said a check was pointing at nothing.
**The "not a rollback" note described a race, and there is no race.** Four
places said the parts the driver could resolve "had already reached the master"
when the unknown name threw — which reads as though a bad part early in the
list, or a low concurrency, would limit the damage. It does not.
`TExecuteBatchCommand` collects the sub-requests into callbacks, runs them all
through `CancelableRunWithBoundedConcurrency`, and only then calls
`.ValueOrThrow()` on the collected list, discarding every result together.
Dispatch is never aborted. Probed five ways: `[create, frobnicate]` created its
node; so did `[frobnicate, create]` with the bad part first;
`[create, frobnicate, create]` created both; and at `concurrency=1`,
`[frobnicate, create, create]` created both and eight creates before a
`frobnicate` created all eight. Every part runs; the failure only destroys the
answers.
The distinction that does bound the damage was nowhere, and it is the one fact
that lets a caller read a 400 at all: a batch refused while its parameters are
being parsed runs **nothing** (`concurrency=0` → `Validation failed at
/concurrency`; a part missing `command`, a part whose `parameters` are not a
dict, and a non-list `requests` → `Error loading parameter /requests`; a missing
`requests` → `Missing required parameter /requests` — a `create` in each of
those requests left no node behind). A batch that reaches execution applies all
of it. Both halves now in AGENTS.md and the rustdoc.
**`BatchInterrupted`'s Display contradicted its own rustdoc.** It rendered "the
batch stopped after 2 of 5 parts, which are already applied on the cluster",
which is wrong twice: `answered` holds `Err` entries that applied nothing, and
per the above the failed request's parts apply whether or not it answers for
them. The variant's rustdoc said exactly that; the one-liner that reaches logs
and `unwrap()` panics said the opposite, in the sentence that gets a caller to
infer a no-op and corrupt state. Rewritten, and pinned by a test that refuses
the old phrase.
**`is_heavy` was the wrong gate.** The cluster's rule is the command's data
types: it throws `cannot be part of a batch since it has inappropriate output
type` before any part runs. Taken from the registry the cluster serves at
`GET /api/v4` (190 commands) and confirmed name by name — a part is refused when
its output type is `tabular` or `binary`, or its input type is `binary`. That is
21 names against `HEAVY`'s 7, and the two differ in both directions:
`get_job_spec` is `isHeavy` and is accepted as a part, while `alter_query` and
`push_queue_producer` are not heavy and are refused. `select_rows` and
`lookup_rows` were sailing through the old gate and are exactly what a caller
would try to batch — `[create x1, select_rows]` is answered 400 with `x1`
created anyway. `NOT_A_BATCH_PART` is the check now, documented as a snapshot of
one registry rather than a promise. `HEAVY` keeps a narrower job: this crate's
own policy that bulk data does not travel inline in a batch body to a light
proxy. That policy is now labelled as the crate's, because the old message
claimed the cluster fails the whole batch over a heavy part and it does not — a
`write_table` part was accepted and wrote its rows.
**`Output` read the v3 registry rows.** `remove`, `remove_tree` and
`set_attribute` were classified `Null`, which is the `ApiVersion3` row; on v4 —
the version this crate speaks — they are `Structured`, and the cluster's own
`/api/v4` says so beside `/api/v3`. Measured one part apiece: `create` →
`{output={node_id=…}}`, `set` → `{output={}}`, `remove` → `{output={}}`,
`exists` → `{output={value=%false}}`. No modelled command answers a bare `{}` on
v4 — which also means the guard added last round was dead against its own
motivating scenario, since it refused only a bare `{}` while the shape a v4
cluster really produces is `{output={}}`, and that is a legitimate `set` success
no output-type bit can tell from a broken `create`. The classification is now
the finer fact every method already documents, the key the success carries: a
`create` is held to `node_id` and a `get` to `value` however the answer is
wrapped, and `set`, `remove` and `raw` have no key to be held to. A `create`
answering `{output={}}` is refused where it used to reach `answer["node_id"]`
and panic a frame away.
Two smaller things. Both rustdocs claimed a two-`create` batch under a fresh id
got two `501`s; `BatchRequest::create` sends `ignore_existing`, so it does not —
measured, a fresh id returns ids identical to the first send, which looks
exactly like a deduplicated replay and is not one. The contrast only holds with
`create_table`, which `examples/batch.rs` already used and explained. And the
mutation-id-per-request property had a single assertion covering it, deep inside
a split test: an A,B,A recycling implementation passed the whole workspace with
that one line deleted. It now has a test of its own, over five chunks.
# Conflicts: # AGENTS.md # crates/ytsaurus-client/src/error.rs # docs/sdk-comparison.md
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.
Closes #11.
What
BatchRequest(newsrc/batch.rs) +Client::execute_batch— the cluster'sexecute_batchwith per-partResults:execute_batch(&BatchRequest) -> Result<Vec<Result<YsonValue>>>. EachOkis that part's own v4 envelope; eachErris aClientError::Clusternamed after the part's command, flattened outer+innermost.create,create_table,exists,get,list,remove,remove_tree,set_attribute) sending byte-identical parameters to theirClientnamesakes, plusraw/raw_with. The reason is the retry class: it turns on what the parts are, and a builder knows where a slice would have to guess conservatively for everyone.Freely; modelled mutations →WithMutationId; any raw part →Neverunless the caller supplies the class throughraw_with. Safe because the driver derives per-part ids from the batch's own id (GetOrGenerateMutationId+GenerateNextBatchMutationId, read first-hand inetc_commands.cpp/rpc/helpers.cpp).concurrency(server-side) andwith_max_part_size(client-side split). Results stitch in part order, one mutation id per request.transaction_idon this command and the node escapes the transaction. Measured;execute_batchjoinedNO_TRANSACTION.Client::execute_batch_withtakes a caller-suppliedMutationIdfor the crash-replay story, and refuses one on a batch that would split (one id cannot cover several requests).ClientError::BatchInterrupted { answered, parts, cause }carries the prefix that already came back when a later chunk fails wholesale, instead of discarding it.What the cluster actually does, measured
Three things this branch asserted before measuring, then corrected:
[create a1, frobnicate]→ 400, no per-part results, anda1exists. Bad part first, bad part last,concurrency=1, eight creates before the bad one — every part applies, always. Dispatch is never aborted: sub-requests are collected into callbacks and run byCancelableRunWithBoundedConcurrency; the unknown name throws inside one executor and.ValueOrThrow()discards the whole result list. Documenting this as a race under-warned — a reader would conclude a bad part early in the list limits the damage. It does not.concurrency=0, a part missingcommand, a part whoseparametersis not a dict → validation errors with zero effect. That distinction is the only thing that lets a caller reason about a 400.write_tablewas accepted as a part and wrote its rows (row_count0→2);get_job_speccame back as an ordinary per-part error. The crate still refuses these, but now says the refusal is its own policy. The real cluster rule is the output/input data type (Command %Qv cannot be part of a batch since it has inappropriate output type), so the gate is nowNOT_A_BATCH_PART, derived from the registry the proxy serves atGET /api/v4(190 commands) rather than from C++ headers — which is also how theOutputclassification was corrected, having first been read off the ApiVersion3 rows.Acceptance criteria → tests
per_part_results_keep_their_order_and_their_sides(live-captured fixture) andone_part_fails_and_the_rest_succeed_in_order, the latter through the fullClientwire path.the_retry_class_is_the_most_cautious_part;a_batch_with_a_raw_part_is_sent_once_whatever_the_policy_saysforces a retriable 503 against a 5-attempt policy, so aFreelymisclassification really fails it (mutation gaveleft: 5, right: 1).a_retried_batch_keeps_its_mutation_id_and_admits_to_the_replay, params decoded fromX-YT-Parametersrather than matched as text.a_big_batch_is_split_and_the_results_stitched_back_in_order(3 requests sized 2/2/1, per-chunk ids pairwise distinct, failure at its own index) plusevery_request_of_a_split_batch_gets_its_own_mutation_id.Decode, count mismatch included; acreateanswering{}names the panic it would have caused.a_split_batch_that_stops_hands_back_the_parts_that_already_applied.Validation
Four independent passes: a critic, a test validator, and two verification rounds over the fix rounds. Every load-bearing claim was mutation-tested; the validator measured "one round trip" with a counting TCP relay rather than inferring it — 12 creates = 1 request, 9.16 ms against 140.77 ms for twelve individual calls — and reproduced the strongest claim live: replay under one explicit mutation id with
retry=%truereturned the identical node id pair, where a fresh id returned two501 already exists.cargo fmt --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspace(663 passed),cargo test --workspace --doc(67 passed): all clean.examples/batch.rsis self-checking and passes against a local cluster.Follow-up candidates (out of scope)
docs/sdk-comparison.mdinstead.NOT_A_BATCH_PARTis a snapshot of one cluster's registry and will drift. Since the proxy serves/api/v4, both this list andHEAVYcould be derived from the live registry at connect time.dispatchreturns on anX-YT-Errorheader before the body is read, so that connection is not pooled — against AGENTS.md's "Connections" rule, and affecting every command equally. Separately, the registry reports 41is_heavycommands whereHEAVYlists 7.