Skip to content

[DIV-08] Error classification relies partly on message-string matching #444

Description

@s2x

Risk for enterprise users: Low
Labels: semantic-divergence, area/retry, technical-debt
Reference implementations: client-go config/retry/config.go (17 typed backoffers) and config/retry/backoff.go

What differs

ErrorClassifier::classify() prefers a typed ErrorKind when a RegionException carries one, but falls back to str_contains() against the exception message for everything else — matching on literals such as 'RaftEntryTooLarge', 'EpochNotMatch', 'ServerIsBusy' and 'NotLeader' (src/Client/Retry/ErrorClassifier.php:32-95). Anything unmatched returns null and is treated as fatal (:97).

client-go classifies from typed protobuf error variants throughout and selects one of 17 backoffer configurations (config/retry/config.go:120-140), each with its own base, cap and jitter strategy.

Why it matters

A porter will assume the retry taxonomy is as complete and as stable as client-go's. Two practical differences follow. First, the default for an unrecognised error is fatal rather than retryable, so any TiKV error the classifier does not know about fails immediately rather than being retried — a newer TiKV version introducing an error variant will produce hard failures rather than degraded-but-working behaviour. Second, message-based matching is fragile: TiKV wording changes, and a locale or formatting difference silently reclassifies an error.

The typed path is good and covers the common cases; this is a robustness concern about the fallback, not a present-day breakage. The backoff parameters themselves align well with client-go's (ServerBusy 2000/10000 and TxnLock 100–200/3000 match closely).

Workaround available today

None needed for known errors. Monitor for unexpected fatal classifications when upgrading TiKV.

Implementation sketch

Extend RegionErrorHandler so every errorpb.Error variant present in src/Proto/Errorpb/ maps to an ErrorKind, eliminating the need for the message fallback on the region-error path. Add the missing backoffer types to reach parity with client-go's set — BoPDRPC, BoTxnNotFound, BoTxnLockFast and BoCommitTSLag have no equivalent in src/Client/Retry/BackoffType.php. Reconsider the default for unrecognised errors: a bounded retry is safer than immediate failure for an unknown region error.

Effort estimate

Small — under a day for the additional backoffer types; Medium for exhaustive typed error mapping.

Acceptance criteria

  • Every errorpb.Error variant maps to a typed ErrorKind.
  • Message-string matching is removed from the region-error path.
  • BoPDRPC, BoTxnNotFound, BoTxnLockFast and BoCommitTSLag equivalents exist.
  • Unrecognised region errors get a bounded retry rather than immediate failure.
  • Tests assert classification for each error variant.

Filed from the Feature gaps vs official clients audit (audit-feature-gaps.md), finding DIV-08.
See the consolidated summary for the root-cause grouping this belongs to.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    auditAdded by audit issue importfeature-gapAdded by audit issue importseverity:lowAdded by audit issue import

    Type

    No type

    Projects

    No projects

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions