*: split ExtractedErrors into MultipleKeyErrors and MultipleRegionErrors#557
*: split ExtractedErrors into MultipleKeyErrors and MultipleRegionErrors#557eduralph wants to merge 5 commits into
Conversation
check_txn_status matched only ExtractedErrors, but its plan delivers per-key errors as MultipleKeyErrors, so the rollback_if_not_exist escalation in get_txn_status_from_lock was unreachable and an orphaned secondary lock poisoned its key permanently. Accept both wrappers. Refs tikv#531 Signed-off-by: Eduard Ralph <eduard@ralphovi.net>
Add a regression test for the MultipleKeyErrors fix: an expired lock whose
primary was never written must escalate to rollback_if_not_exist and resolve,
rather than surfacing the raw error. The test asserts the two CheckTxnStatus
calls and fails on the pre-fix code with MultipleKeyErrors([KeyError { .. }]).
Also correct the new comment: the key errors are produced by
single_shard_handler, not single_plan_handler, and name CollectSingle as the
reason this call site differs from the plans that never see MultipleKeyErrors.
Refs tikv#531
Signed-off-by: Eduard Ralph <eduard@ralphovi.net>
`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>
ti-chi-bot
Bot
added
do-not-merge/work-in-progress
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
✨ 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 |
ti-chi-bot
Bot
added
contribution
ti-chi-bot
Bot
removed
the
do-not-merge/work-in-progress
1 participant
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.