*: re-vendor kvproto at b41e863 (2026-06-22) and record the vendored revision#550
Merged
Merged
Conversation
ti-chi-bot
Bot
added
dco-signoff: yes
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe update vendors a newer kvproto revision, expands protobuf service and message contracts, standardizes code-generation options, and changes Rust retry and lock handling for undetermined results and shared-lock wrappers. ChangesProtocol and transaction safety changes
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
eduralph
force-pushed
the
kvproto-refresh
branch
4 times, most recently
from
July 18, 2026 12:00
ccb481a to
0bc204c
Compare
Contributor
Author
|
/retest |
|
@eduralph: Cannot trigger testing until a trusted user reviews the PR and leaves an DetailsIn response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
eduralph
force-pushed
the
kvproto-refresh
branch
from
July 18, 2026 12:37
0bc204c to
713dc42
Compare
Collaborator
|
Suggest to split the handling of So that this current PR focuses only on re-vendor kvprotos. |
…revision The vendored protos were an unrecorded late-2023/early-2024 vintage (errorpb and pdpb untouched since tikv#324, one hand-patched addition from tikv#519). Nothing recorded which kvproto revision they came from, and the staleness now blocks real features. Re-vendor everything at pingcap/kvproto b41e86365ce0 — the revision client-go currently pins — and add proto/VERSION so the vintage is never again guesswork. Regenerated with the existing tonic-build pipeline (21 files). One mechanical source fix: pdpb.RequestHeader gained caller_component/caller_id, so the TSO stream's header construction now defaults the new fields. This commit is the re-vendor and nothing else. Newly available surface includes KvFlush / KvBufferBatchGet (pipelined DML), HealthFeedback, and pdpb.BatchScanRegions. Two new protocol semantics become VISIBLE with the refresh — errorpb.UndeterminedResult (an unknown raft apply outcome) and shared locks (SharedLock / SharedPessimisticLock, whose real holders live in shared_lock_infos). Handling them is deliberately left to follow-up PRs so this one stays a pure re-vendor. Neither is a regression introduced here: - UndeterminedResult falls through to the generic region-error arm, which backs off and retries — the same default the follow-up keeps for every plan that is not a commit point. - A shared-lock wrapper carries no key bytes on the wire regardless of which proto vintage parses it, so the keyspace truncation hazard predates this commit; the refresh only makes the wrapper identifiable. Tests: make check green; 67 lib tests; txn/raw/failpoint integration suites green against a local api-v2 cluster. Signed-off-by: Eduard R. <eduard@ralphovi.net>
eduralph
force-pushed
the
kvproto-refresh
branch
from
July 25, 2026 13:27
adfed61 to
d7e581e
Compare
This was referenced Jul 25, 2026
Contributor
Author
Done — thanks, that's the right call. #550 is now the re-vendor and nothing else (59 files: the The two semantics are split out into #555 (undetermined result) and #556 (shared locks), Note the branch is now rebased onto master rather than carrying your Worth stating explicitly, since it's the thing that would justify keeping them together: neither
So #550 is safe to merge on its own timeline. |
eduralph
added a commit
to getwyrd/client-rust
that referenced
this pull request
Jul 25, 2026
…ints
The re-vendored kvproto exposes errorpb.UndeterminedResult: the server telling
us the raft apply outcome is UNKNOWN. Today it falls through to the generic
region-error arm and is retried like any transient failure. For most requests
that is right, but not for a commit point, where "failed" must never be claimed
for a transaction that may already be durable.
Follow client-go's shape: its transport never retries UndeterminedResult
("should not retry ... processed by the caller", region_request.go) and each
ACTION decides. So the plan layer keeps RETRY as the default — replaying an
idempotent request resolves the uncertainty, and on backoff exhaustion the
error escapes UNCHANGED so callers can still classify it — while plans for
which a replay is unsafe opt in to terminal-on-first-sight via
retry_multi_region_terminal_on_undetermined:
- raw CAS: a replay would compare against its OWN effect and report
succeed = false for a write that happened.
- the primary commit: client-go returns ErrResultUndetermined here (commit.go)
rather than retrying.
- async-commit / 1PC prewrites: the prewrite IS the commit point. Stricter
than client-go, in the safe direction.
Shard aggregation is fixed alongside: collect::<Result<_>>() returns the first
error BY SHARD INDEX, so a lower-index shard's determinate error could mask a
higher-index shard's UndeterminedResult and tell the caller the request
definitely failed. An undetermined error from any shard now wins.
Commit paths classify the surfaced error as UndeterminedError. Two places are
deliberately conservative and say so in comments: a 1PC prewrite that sharding
downgraded to 2PC, and an undetermined error arriving from the commit plan's
resolve_lock layer rather than the commit dispatch. Both only over-report
uncertainty, which makes the caller verify instead of replay.
Split out of the kvproto re-vendor at pingyu's suggestion (tikv#550).
Tests: 4 new — terminal for opted-in plans under a generous backoff (proving
terminal, not retry-exhausted), preserved unchanged on retry exhaustion,
recognizer positive/negative, and the shard-masking hazard. 71 lib tests green.
Signed-off-by: Eduard R. <eduard@ralphovi.net>
eduralph
added a commit
to getwyrd/client-rust
that referenced
this pull request
Jul 25, 2026
…ints
The re-vendored kvproto exposes errorpb.UndeterminedResult: the server telling
us the raft apply outcome is UNKNOWN. Today it falls through to the generic
region-error arm and is retried like any transient failure. For most requests
that is right, but not for a commit point, where "failed" must never be claimed
for a transaction that may already be durable.
Follow client-go's shape: its transport never retries UndeterminedResult
("should not retry ... processed by the caller", region_request.go) and each
ACTION decides. So the plan layer keeps RETRY as the default — replaying an
idempotent request resolves the uncertainty, and on backoff exhaustion the
error escapes UNCHANGED so callers can still classify it — while plans for
which a replay is unsafe opt in to terminal-on-first-sight via
retry_multi_region_terminal_on_undetermined:
- raw CAS: a replay would compare against its OWN effect and report
succeed = false for a write that happened.
- the primary commit: client-go returns ErrResultUndetermined here (commit.go)
rather than retrying.
- async-commit / 1PC prewrites: the prewrite IS the commit point. Stricter
than client-go, in the safe direction.
Shard aggregation is fixed alongside: collect::<Result<_>>() returns the first
error BY SHARD INDEX, so a lower-index shard's determinate error could mask a
higher-index shard's UndeterminedResult and tell the caller the request
definitely failed. An undetermined error from any shard now wins.
Commit paths classify the surfaced error as UndeterminedError. Two places are
deliberately conservative and say so in comments: a 1PC prewrite that sharding
downgraded to 2PC, and an undetermined error arriving from the commit plan's
resolve_lock layer rather than the commit dispatch. Both only over-report
uncertainty, which makes the caller verify instead of replay.
Split out of the kvproto re-vendor at pingyu's suggestion (tikv#550).
Tests: 4 new — terminal for opted-in plans under a generous backoff (proving
terminal, not retry-exhausted), preserved unchanged on retry exhaustion,
recognizer positive/negative, and the shard-masking hazard. 71 lib tests green.
Signed-off-by: Eduard R. <eduard@ralphovi.net>
eduralph
added a commit
to getwyrd/client-rust
that referenced
this pull request
Jul 25, 2026
The re-vendored kvproto exposes shared locks (Op::SharedLock / Op::SharedPessimisticLock). Their contract is unusual: a shared lock's real holders live ONLY in kvrpcpb.LockInfo.shared_lock_infos — the proto comment is explicit that you must "DO NOT read from the wrapper LockInfo", whose own key and lock_version are unset. This client does not implement shared-lock resolution, and every partial handling of one is worse than none: resolving the wrapper checks transaction 0; filtering on the wrapper's fields (use_async_commit, lock_type) silently drops the real members; and the pessimistic-lock special cases in the resolver do not know SharedPessimisticLock. So resolution REFUSES them with an explicit error — in resolve_locks, in LockResolver's crate-public entry point, and in CleanupLocks::execute before any filter runs. Until real support lands, an explicit error is the only answer that cannot roll back a live transaction or skip a dead one. Servers that predate shared locks never produce them, so this is a no-op there. The API-v2 keyspace codecs are made shared-lock-aware separately, because they run on scan results that never reach the resolver. They now take the shape of client-go's codecV2.decodeLockInfo (internal/apicodec/codec_v2.go), which decodes a LockInfo's own Key/PrimaryLock/Secondaries and then recurses into SharedLockInfos — it has no wrapper special case. This client now does the same: convert the lock's OWN key fields, then recurse into the members. An earlier revision skipped a wrapper's own fields entirely, reading the proto's "DO NOT read from the wrapper LockInfo" as covering them. Checking the writer (TiKV's SharedLocks::into_lock_info in components/txn_types/src/lock.rs, identical at v8.5.7 and on master) shows that is only half right, and the half it gets wrong matters: info.set_shared_lock_infos(shared_locks.into()); info.set_key(raw_key); A wrapper sets lock_type, shared_lock_infos and key — the key it locks — and leaves primary_lock and lock_version at their defaults; each member is built from that SAME raw key plus its own primary and version. So the caveat scopes the per-transaction fields, not the locked key. Skipping the wrapper wholesale would have handed scan_locks a physical key beside decoded member keys, while converting it wholesale would have hit the length assertion on the unset primary. Both fields are exercised by tests. The two directions guard differently, on purpose. Truncating, empty bytes can only mean "unset" — an encoded key always carries its 4-byte prefix — so unset fields are skipped rather than run into pretruncate_bytes' length assertion. Encoding, the input is a LOGICAL key and the empty logical key is valid in API v2, so nothing is skipped: scan_locks -> resolve_locks round-trips locks through truncate-then-encode, and skipping empties there would strand a lock on the empty key with no prefix and send resolution after an empty physical key. Wrappers need no encode-side guard because resolution refuses them first. That panic hazard predates the re-vendor: a wrapper arrives the same way whichever proto vintage parses it. Full shared-lock support is deliberately follow-up work. Split out of the kvproto re-vendor at pingyu's suggestion (tikv#550). Tests: 4 new — the resolver refusal (a plain lock passes; a wrapper carrying members and a lock marked shared only by its op are both refused); the codec converting a wrapper's own key alongside its members; a wrapper's unset fields surviving truncation (the other reading, so both are pinned); and a lock on the empty logical key round-tripping through truncate-then-encode. 71 lib tests green. The coverage is unit-level by necessity: CI pins TIKV_VERSION v8.5.5, which predates shared locks, so no integration test in this repo can produce one, and the refusal path is unreachable against that server. v8.5.6 is the first release carrying shared locks, so a CI pin bump within the same release family would make these paths reachable — left to a separate change. Signed-off-by: Eduard R. <eduard@ralphovi.net>
ti-chi-bot
Bot
added
needs-1-more-lgtm
Collaborator
YuJuncen
approved these changes
Jul 26, 2026
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: pingyu, YuJuncen The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
3 participants
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.
Why: the vendored protos are an unrecorded late-2023/early-2024 vintage (
errorpb.proto/pdpb.protountouched since #324, one hand-patched addition from #519). Nothing records whichkvproto revision they came from, and the staleness now blocks real features.
What: all vendored protos + the include tree re-vendored at pingcap/kvproto
b41e863— therevision client-go pins today — plus a new
proto/VERSIONrecording it. Regenerated with theexisting tonic-build pipeline (21 files); one mechanical source fix (
pdpb.RequestHeadergainedcaller_component/caller_id). Newly available surface includesKvFlush/KvBufferBatchGet(pipelined DML),
HealthFeedback, andpdpb.BatchScanRegions.Scope. Per @pingyu's review, this PR is now the re-vendor and nothing else. The two new
protocol semantics that become visible with the refresh are split into follow-ups stacked on this
branch — #555 (
errorpb.UndeterminedResult) and #556 (shared locks). Neither is aregression introduced here: an undetermined result falls through to the generic region-error arm and
retries (the same default the follow-up keeps for non-commit-point plans), and a shared-lock wrapper
carries no key bytes on the wire whichever proto vintage parses it, so the keyspace hazard predates
this PR.
Verification:
make checkgreen (including a clean re-run of the proto-build pipeline — thecommitted bindings match what it generates); 67 lib tests; txn/raw/failpoint integration suites green
against a local api-v2 cluster; and behaviour compared against client-go by driving both clients
through identical transactional and raw scenarios — no observable change.