Repository navigation
*: split ExtractedErrors into MultipleKeyErrors and MultipleRegionErrors - #557
Merged
Merged
Conversation
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 42.86% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 4 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly identifies the main change: replacing ExtractedErrors with separate MultipleKeyErrors and MultipleRegionErrors variants. The leading '*:' is minor noise but does not reduce clarity. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
- Fix all pre-merge checks with AI
Warning
Some tools did not complete. Review the errors below.
🔧 Clippy (1.98.1)
-
Clippy execution failed
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 @coderabbitai help to get the list of available commands.
ti-chi-bot
Bot
added
contribution
eduralph
marked this pull request as ready for review
July 25, 2026 22:16
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
`ExtractedErrors` carried either key errors or region errors, so a caller had to pop an element and type-test it to learn which it was holding. Worse, WHICH variant reached the caller was decided by plan shape rather than by what the server reported: without a collapsing merge, `ExtractError` normalized the per-shard `Vec<Result<_>>` into `ExtractedErrors`; with `CollectSingle`, the merge popped the single `Result` out first, so `MultipleKeyErrors` became the plan's own error and `ExtractError` re-raised it without ever calling `key_errors()`. That is not a theoretical hazard. It is the bug fixed in the parent commit, where `check_txn_status` matched only one variant and an orphaned lock whose primary was never written stayed unresolved forever. It is also visible in the public raw API today: `delete` and `put_with_ttl` report key errors as `MultipleKeyErrors` while `batch_delete`, `batch_put_with_ttl` and `delete_range` report them as `ExtractedErrors`, for the same class of failure and with nothing in either signature to suggest it. Remove `ExtractedErrors`. `ExtractError` now reports key errors as `MultipleKeyErrors` and region errors as a new `MultipleRegionErrors`, so the variant names what the server said and is independent of how the plan was composed. The parent commit's two-variant match arm collapses back to one. This also fixes a leak in `CleanupLocks::execute`, which popped the last error to type-test it and, on the key-error path, assigned the REMAINING vec to `key_error` — dropping a lone key error and leaving `key_errors()` to report `Some(vec![])`, an error carrying no errors. With the kind known from the variant, neither arm pops to decide. Breaking: `Error::ExtractedErrors` is removed. Callers matching it should match `Error::MultipleKeyErrors` (per-key failures) or `Error::MultipleRegionErrors` (region failures); callers that inspected the payload's type to tell them apart can now match the variant instead. One wrinkle the split exposes rather than creates: `HasKeyErrors for Result<T, Error>` reports ANY per-shard `Err` as a key error, which is how a plan keeping its `Vec<Result<_>>` carries a hard failure through. Naming the variant `MultipleKeyErrors` would then be a lie for a gRPC or exhausted-retry error, so `ExtractError` wraps only what the RESPONSE reported and propagates anything else unchanged. "Reported by the response" means the two error types the `HasKeyErrors` response impls construct: `KeyError` from a `kvrpcpb::KeyError` field, and `KvError` from a raw endpoint's string `error` field. Both must count — testing for `KeyError` alone would leave raw operations plan-shape-dependent, which is the very thing being removed. No current plan composes `retry_multi_region_preserve_results` with `extract_error`, so this changes no existing behaviour; it keeps the new variant honest for the compositions that could. Tests: 4 new. One pins the invariant this buys — a response key error surfaces as `MultipleKeyErrors` both with and without a collapsing merge, so recomposing a plan can no longer flip the variant under a distant match arm. The other pins the region-error side on the shape that actually reaches `ExtractError`: a plan with no retry layer above it, as in `resolve_lock_with_retry`, since `RetryableMultiRegion` consumes region errors itself. The third pins that a hard per-shard error keeps its own class instead of being relabelled, and the fourth that a raw endpoint's string error is classified the same way under either plan shape. 72 lib tests green; txn/raw/failpoint integration suites green against a local api-v2 cluster. Signed-off-by: Eduard R. <eduard@ralphovi.net>
eduralph
force-pushed
the
error-variants
branch
from
October 17, 2026 16:23
6908ad3 to
ea84837
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
ti-chi-bot
Bot
added
needs-1-more-lgtm
Collaborator
|
/cc @ekexium |
ekexium
approved these changes
Oct 19, 2026
ti-chi-bot
Bot
added
lgtm
and removed
needs-1-more-lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ekexium, pingyu 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
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.
Follows @pingyu's suggestion on #544. Thanks for it — the problem turned out to be a bit worse than
redundancy.
Why.
ExtractedErrorscarried either key errors or region errors, so a caller had to pop anelement and type-test it to learn which it was holding. Worse, which variant reached the caller was
decided by plan shape, not by what the server reported:
ExtractErrorsees the per-shardVec<Result<_>>and normalizes toExtractedErrors;merge(CollectSingle)→ the merge pops the singleResultout first, soMultipleKeyErrorsbecomes the plan's own error and
ExtractErrorre-raises it without ever callingkey_errors().That is exactly the bug #544 fixes, and it is already visible in the public raw API for the same
class of failure:
delete,put_with_ttlretry_multi_region→merge(CollectSingle)→extract_errorMultipleKeyErrorsbatch_delete,batch_put_with_ttl,delete_rangeretry_multi_region→extract_errorExtractedErrorsWhat.
ExtractedErrorsis removed.ExtractErrornow reports key errors asMultipleKeyErrorsand region errors as a new
MultipleRegionErrors, so the variant names what the server said and isindependent of how the plan was composed. #544's two-variant match arm collapses back to one.
A latent bug this removes.
CleanupLocks::executepopped the last error to type-test it and, onthe key-error path, assigned the remaining vec to
key_error— so a lone key error was dropped andkey_errors()went on to reportSome(vec![]), an error carrying no errors. With the kind knownfrom the variant, neither arm pops to decide.
One wrinkle the split exposes rather than creates.
HasKeyErrors for Result<T, Error>reportsany per-shard
Erras a key error — that is how a plan keeping itsVec<Result<_>>carries a hardfailure through. Naming the variant
MultipleKeyErrorswould then be a lie for a gRPC orexhausted-retry error, so
ExtractErrorwraps only what the response reported and propagatesanything else unchanged. "Reported by the response" means the two error types the
HasKeyErrorsresponse impls construct:
KeyErrorfrom akvrpcpb::KeyErrorfield, andKvErrorfrom a rawendpoint's string
errorfield — both count, since testing forKeyErroralone would leave rawoperations plan-shape-dependent, which is the very thing being removed. No current plan composes
retry_multi_region_preserve_resultswithextract_error, so this changes no existing behaviour; itkeeps the new variant honest for the compositions that could.
Breaking.
Error::ExtractedErrorsis removed. Callers matching it should matchError::MultipleKeyErrors(what the server said about the request) orError::MultipleRegionErrors(region failures); callers that inspected the payload's type to tell them apart can now match the
variant instead. Per @pingyu's note on #544, the next release looks like a semver-major anyway.
Verification. 4 new tests. One pins the invariant this buys — a response key error surfaces as
MultipleKeyErrorsboth with and without a collapsing merge, so recomposing a plan can no longer flipthe variant under a distant match arm. One pins the region side on the shape that actually reaches
ExtractError(a plan with no retry layer above it, as inresolve_lock_with_retry, sinceRetryableMultiRegionconsumes region errors itself). One pins that a hard per-shard error keeps itsown class. One pins that a raw endpoint's string error is classified the same way under either plan
shape. 72 lib tests;
make checkgreen; txn/raw/failpoint integration suites green against a localapi-v2 cluster.
Summary by CodeRabbit
Bug Fixes
Improvements