refactor(storage): one typed-accessor skeleton — ValueKind/OwnedKind generics replace Database's per-type accessor matrix (W5)#400
Merged
Conversation
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
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 Run ID: 📒 Files selected for processing (6)
✨ 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 |
TinDang97
added a commit
that referenced
this pull request
Jul 20, 2026
… disambiguation, #48 RFC tie-in (W6) Closes out the storage-unification campaign's F6 hygiene tail: - persistence::aof_manifest::ShardManifest -> AofShardManifest. Two unrelated structs shared the name ShardManifest: the page manifest's (persistence::manifest — the durable spill/offload root every eviction/spill call site threads around) and the AOF manifest's per-shard entry. Same name, different on-disk formats, different planes — ambiguous in code search and in every cross-plane persistence discussion (the unified-manifest RFC lists this rename as a prerequisite). The AOF struct is private to its module tree (zero external importers), so the rename is contained to aof_manifest/{mod,shard_rewrite}.rs; a doc note on the struct records the collision rationale. - storage::tier::ResidencyTier doc note: disambiguates it from persistence::manifest::StorageTier — StorageTier is an ON-DISK FileEntry field with persisted byte values; ResidencyTier is the IN-MEMORY residency vocabulary (K4 types-only module, parked by design for the M4 tiering milestone). They intersect in concept but not in role and must not be merged. tier.rs itself stays: it is the documented K4 forward-vocabulary the vector-tiering-v2 plan adopts, not accidental dead code. - .planning/rfcs/unified-manifest-engine.md: adds the task #48 unified poison-record policy tie-in — a future manifest engine's section decoders must adopt the replication policy verbatim (fail-closed per section with an INFO counter, never skip-and-continue past an undecodable section). No behavior change; rename + docs only. Gates: lib suite 4427 green; clippy -D warnings on default and tokio,jemalloc feature sets; fmt. Stacked on refactor/w5-typed-accessors (PR #400). author: Tin Dang
TinDang97
force-pushed
the
refactor/w5-typed-accessors
branch
from
July 20, 2026 05:33
cdd5f8b to
247d776
Compare
TinDang97
force-pushed
the
refactor/w4-eviction-api
branch
from
July 20, 2026 05:33
e1df75f to
888e46b
Compare
…generics replace Database's per-type accessor matrix (W5)
Database had grown a ~20-method typed-accessor matrix: get_X,
get_or_create_X, and get_X_ref_if_alive for each container type (hash,
list, set, sorted set, stream), every one a copy-paste of the same
skeleton — expiry check -> cold promote/read-through -> compact-
encoding upgrade -> variant projection — with only the enum arms
differing. Skeleton fixes (the P0 cold-collection-visibility fix, the
promote-before-fabricate fix) had to be applied N times and could
silently miss a copy.
The skeleton now exists ONCE per access shape as a generic on
Database:
get_ref_if_alive::<K> &self; hot probe -> expiry -> classify ->
non-promoting cold read-through (Owned view)
get_or_create::<K> &mut; expiry-drop -> cold promote ->
insert-on-miss -> upgrade -> project mut
get_promoted::<K> &mut; expiry-drop -> cold promote ->
upgrade -> project shared
Everything genuinely per-type — which enum variants map to which view,
how a compact encoding upgrades (incl. the deliberate asymmetries:
sorted sets upgrade the legacy BTreeMap form but NOT SortedSetListpack,
whose upgrade lives in the zset command layer), what a fresh entry
looks like — lives in the new storage::db_kind module as
ValueKind/OwnedKind impls on zero-sized markers. Static dispatch only:
every K is a concrete marker resolved at compile time, so the
generated code is identical to the hand-written originals.
The public per-type methods (get_hash, get_or_create_set,
get_list_ref_if_alive, get_stream_if_alive, ...) remain as thin
delegators — command-layer call sites are untouched. The generic
get_or_create also replaces the per-type unwrap-after-insert with the
defensive log-and-error path only the hash accessor previously had.
Deleted: the four caller-less get_X_if_alive variants (hash/list/set/
sorted_set; compact encodings returned None — long superseded by the
_ref_if_alive family). get_or_create_intset and the *_listpack
threshold accessors keep their bespoke shapes (their Option-returning
contracts don't fit the trait and have single call sites).
Pure refactor — no behavior change:
- Behavior pins added GREEN BEFORE the refactor and re-verified after:
hot-WRONGTYPE through all four ref accessors; a hot HashWithTtl entry
maps to HashRef::WithTtl carrying the CALLER's now_ms (expired field
filters, live field reads through). Cold-Owned legs, promote-instead-
of-fabricate, and wrongtype pins already existed and still pass.
- Net -318 lines; db.rs 3431 -> 3113 lines.
Gates: lib suite 4427 green (4425 + 2 new pins) on macOS + Linux VM
release-fast, monoio + tokio,jemalloc feature sets; crash matrix +
kill-9 offload + cold-visibility suites on Linux VM; clippy -D
warnings on both feature sets; fmt.
Stacked on refactor/w4-eviction-api (PR #399).
author: Tin Dang
TinDang97
force-pushed
the
refactor/w5-typed-accessors
branch
from
July 20, 2026 05:37
247d776 to
f39c957
Compare
TinDang97
added a commit
that referenced
this pull request
Jul 20, 2026
… disambiguation, #48 RFC tie-in (W6) Closes out the storage-unification campaign's F6 hygiene tail: - persistence::aof_manifest::ShardManifest -> AofShardManifest. Two unrelated structs shared the name ShardManifest: the page manifest's (persistence::manifest — the durable spill/offload root every eviction/spill call site threads around) and the AOF manifest's per-shard entry. Same name, different on-disk formats, different planes — ambiguous in code search and in every cross-plane persistence discussion (the unified-manifest RFC lists this rename as a prerequisite). The AOF struct is private to its module tree (zero external importers), so the rename is contained to aof_manifest/{mod,shard_rewrite}.rs; a doc note on the struct records the collision rationale. - storage::tier::ResidencyTier doc note: disambiguates it from persistence::manifest::StorageTier — StorageTier is an ON-DISK FileEntry field with persisted byte values; ResidencyTier is the IN-MEMORY residency vocabulary (K4 types-only module, parked by design for the M4 tiering milestone). They intersect in concept but not in role and must not be merged. tier.rs itself stays: it is the documented K4 forward-vocabulary the vector-tiering-v2 plan adopts, not accidental dead code. - .planning/rfcs/unified-manifest-engine.md: adds the task #48 unified poison-record policy tie-in — a future manifest engine's section decoders must adopt the replication policy verbatim (fail-closed per section with an INFO counter, never skip-and-continue past an undecodable section). No behavior change; rename + docs only. Gates: lib suite 4427 green; clippy -D warnings on default and tokio,jemalloc feature sets; fmt. Stacked on refactor/w5-typed-accessors (PR #400). author: Tin Dang
pilotspacex-byte
added a commit
that referenced
this pull request
Jul 20, 2026
… disambiguation, #48 RFC tie-in (W6) (#401) Closes out the storage-unification campaign's F6 hygiene tail: - persistence::aof_manifest::ShardManifest -> AofShardManifest. Two unrelated structs shared the name ShardManifest: the page manifest's (persistence::manifest — the durable spill/offload root every eviction/spill call site threads around) and the AOF manifest's per-shard entry. Same name, different on-disk formats, different planes — ambiguous in code search and in every cross-plane persistence discussion (the unified-manifest RFC lists this rename as a prerequisite). The AOF struct is private to its module tree (zero external importers), so the rename is contained to aof_manifest/{mod,shard_rewrite}.rs; a doc note on the struct records the collision rationale. - storage::tier::ResidencyTier doc note: disambiguates it from persistence::manifest::StorageTier — StorageTier is an ON-DISK FileEntry field with persisted byte values; ResidencyTier is the IN-MEMORY residency vocabulary (K4 types-only module, parked by design for the M4 tiering milestone). They intersect in concept but not in role and must not be merged. tier.rs itself stays: it is the documented K4 forward-vocabulary the vector-tiering-v2 plan adopts, not accidental dead code. - .planning/rfcs/unified-manifest-engine.md: adds the task #48 unified poison-record policy tie-in — a future manifest engine's section decoders must adopt the replication policy verbatim (fail-closed per section with an INFO counter, never skip-and-continue past an undecodable section). No behavior change; rename + docs only. Gates: lib suite 4427 green; clippy -D warnings on default and tokio,jemalloc feature sets; fmt. Stacked on refactor/w5-typed-accessors (PR #400). author: Tin Dang Co-authored-by: Tin Dang <tindang.ht97@gmail.com>
TinDang97
added a commit
that referenced
this pull request
Jul 20, 2026
…/kv_ops/accessors (#403) db.rs had grown to 3113 lines against the repo's 1500-line file-size rule. The W5 typed-accessor unification (#400) concentrated the accessor matrix into three generics, which made the module's internal seams clean enough for a mechanical split: - db/mod.rs (1152 lines): Database struct, listpack/intset constants, per-element cost functions, HashTtlCond/FieldState/WrongType, ctor/clock methods, O(1) memory accounting, Default impl, and the whole test module (tests stay in mod.rs per repo convention). - db/hash_ttl.rs (476): HEXPIRE-family per-field TTL storage primitives (hash_set_field_ttl .. cleanup_empty_hash). - db/kv_ops.rs (833): core keyspace ops — get/set/remove families, lazy expiry, cold-tier promotion/read-through, scan/keys/random_key, expiry bookkeeping, versioning/LRU touch. - db/accessors.rs (701): W5 ValueKind/OwnedKind generic skeletons, per-type thin delegators, bespoke intset/listpack accessors, read-only ref accessors, blocking-wakeup list/zset helpers, streams. Pure code motion: every method body moved verbatim; public paths are unchanged (storage::db::Database and the cost fns resolve exactly as before; submodules only add inherent impl blocks). The only signature changes are visibility: check_expired / is_expired / wrongtype_error / cold_contains_alive / cold_read_only became pub(super) so sibling files and mod.rs tests keep access. Submodule files use crate:: imports per the module-structure convention. Gates: lib suite 4427 green (identical count to pre-split main); clippy -D warnings clean on default and tokio,jemalloc feature sets; fmt; VM Linux lib green on both runtimes. author: Tin Dang
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
2 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.
Summary
Fifth PR of the storage-unification campaign (stacked on #399 ← #398 ← #397 ← #396). Fixes finding F3 (Database accessor matrix).
Before
Databasehad a ~20-method typed-accessor matrix —get_X,get_or_create_X,get_X_ref_if_aliveper container type — each a copy-paste of the same skeleton (expiry check → cold promote/read-through → compact-encoding upgrade → variant projection) with only the enum arms differing. Skeleton fixes (the P0 cold-collection-visibility fix, promote-before-fabricate) had to be applied N times and could silently miss a copy.After
The skeleton exists once per access shape as a generic on
Database:get_ref_if_alive::<K>&self; hot probe → expiry → classify → non-promoting cold read-through (Ownedview)get_or_create::<K>&mut; expiry-drop → cold promote → insert-on-miss → upgrade → project mutget_promoted::<K>&mut; expiry-drop → cold promote → upgrade → project sharedEverything genuinely per-type lives in the new
storage::db_kindmodule asValueKind/OwnedKindimpls on zero-sized markers (HashKind,ListKind,SetKind,SortedSetKind,StreamKind) — including the deliberate asymmetries (sorted sets upgrade the legacyBTreeMapform but notSortedSetListpack, whose upgrade lives in the zset command layer). Static dispatch only — generated code identical to the hand-written originals.The 15 public per-type methods remain as thin delegators, so command-layer call sites are untouched. The generic
get_or_createalso replaces the per-typeunwrap()-after-insert with the defensive log-and-error path only the hash accessor previously had. The four caller-lessget_X_if_alivevariants are deleted.get_or_create_intsetand the*_listpackthreshold accessors keep their bespoke shapes (Option-returning contracts, single call sites).Net −318 lines;
db.rs3431 → 3113.Gates
HashWithTtl→HashRef::WithTtlcarrying the caller'snow_ms(expired field filters, live field reads). Pre-existing cold-Owned, promote-instead-of-fabricate, and wrongtype pins all still pass.cold_collection_visibility4/4 ·crash_matrix_cross_plane46/46 ·spill_batch_kill92/2 ·disk_offload_no_aof1/1 on Linux VM with pinned fresh-ELFMOON_BIN-D warnings×2 feature sets · fmt · CHANGELOG entry