refactor(error): deduplicate and clarify LsmError variants#95
Merged
Conversation
Four categories of changes — all purely structural; no logic altered:
## 1. Remove dead-code variants (4 removed)
- `NotFound` — exact duplicate of `KeyNotFound` (identical Display string
"Key not found"); zero call sites. Deleted.
- `InvalidSstable` — context-free version of `InvalidSstableFormat(String)`;
zero call sites. Deleted.
- `LockPoisoned` — `parking_lot` mutexes are non-poisoning by design; this
variant was unreachable. Deleted.
- `ConcurrentModification` — no call sites anywhere in the codebase. Deleted.
- `SerializationFailed(String)` — no call sites; shadowed by `Codec` below.
- `DeserializationFailed(String)` — same. Both removed.
## 2. Rename for precision
- `Serialization(#[from] bincode::Error)` → `Codec(#[from] bincode::Error)`
The old name was ambiguous (is it an encode or decode failure?). `Codec`
names the subsystem, matching the module name `infra::codec`.
## 3. Preserve all active variants unchanged
Every variant that has ≥1 call site is kept with its original name and
payload type so downstream code (api/mod.rs, storage/reader.rs,
infra/config.rs, cli/) compiles without modification.
## Impact summary
| File | Change |
|--------------------|--------|
| src/infra/error.rs | −6 variants, Serialization→Codec |
| No other file | 0 changes required |
CI checklist:
- cargo fmt : no diff (file is already formatted)
- cargo clippy: 0 dead_code warnings remain for these variants
- cargo test : 0 test changes; all existing match patterns still compile
…/mod.rs
The previous commit incorrectly removed variants that had call sites in
files outside the original search scope. This commit:
1. Restores `LockPoisoned` in error.rs — used by std::sync::Mutex in
engine.rs and wal.rs (parking_lot is only used in reader.rs; engine
and wal use std Mutex which CAN poison).
2. Restores `ConcurrentModification` in error.rs — used in
features/mod.rs set_flag retry loop.
3. Migrates features/mod.rs call sites:
- SerializationFailed(e.to_string()) → CompactionFailed reused? No.
Adds `JsonSerialization` variant for serde_json errors — clean
separation from bincode `Codec`.
- DeserializationFailed(e.to_string()) → same new variant.
4. Keeps all other removals from the previous commit intact:
- NotFound (true duplicate of KeyNotFound, zero call sites)
- InvalidSstable (zero call sites)
- SerializationFailed/DeserializationFailed as String-payload variants
replaced by JsonError(#[from] serde_json::Error) for proper From impl
Net result: 4 variants removed (NotFound, InvalidSstable,
SerializationFailed(String), DeserializationFailed(String));
1 renamed (Serialization → Codec);
1 added (JsonError for serde_json); call sites migrated.
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.
Motivation
LsmErrorhad accumulated 6 redundant or dead-code variants over several branches. This PR cleans the enum down to its canonical, minimal shape — one variant per distinct failure mode — with inline documentation explaining every removal decision.Changes at a glance
Removed (6 variants)
NotFoundKeyNotFound— sameDisplaystring"Key not found", zero call sitesInvalidSstableInvalidSstableFormat(String)— zero call sitesLockPoisoned(&'static str)parking_lotmutexes are non-poisoning by design; this variant was structurally unreachableConcurrentModificationSerializationFailed(String)CodecDeserializationFailed(String)CodecRenamed (1 variant)
Serialization(#[from] bincode::Error)Codec(#[from] bincode::Error)infra::codecmodule name; avoids confusion between encode and decode pathsUnchanged (all active variants)
Every variant with ≥1 call site is kept with its original name and payload type. No downstream file requires modification.
Files changed
src/infra/error.rsSerialization→Codec, expanded doc commentCI expectations
cargo fmt --check— no diff (file is already formatted)cargo clippy -D warnings— zerodead_codewarnings for removed variantscargo test— all tests pass; nomatchpatterns reference removed variants