backup: Redis list encoder (Phase 0a)#755
Conversation
Adds the !lst|meta| + !lst|itm| → lists/<key>.json encoder per the
Phase 0 design doc (docs/design/2026_04_29_proposed_snapshot_logical_decoder.md).
Wire format mirrors store/list_helpers.go:
- !lst|meta|<userKey> -> 24-byte (Head, Tail, Len) blob
- !lst|itm|<userKey><seq(8)> -> raw item bytes (binary-safe)
- !lst|meta|d|... -> meta delta. Skipped silently
(same policy as hash deltas):
!lst|itm| keys are the source of
truth at backup time and the
delta arithmetic does not need
to be replayed.
- !lst|claim|... -> POP tombstone for OCC uniqueness.
The live read path (rangeList →
fetchListRange in redis.go:3281)
does NOT consult claims; POPs
Del the underlying item key in
the same OCC commit, so a
snapshot taken after a POP has
no item record for the popped
seq. The encoder therefore skips
claim keys entirely.
Output JSON shape matches the design's other wide-column types:
{"format_version": 1,
"items": [..., {"base64":"..."}, ...],
"expire_at_ms": null | <ms>}
Items are emitted in ascending sortable-int64 seq order, which is the
live store's left-to-right read order (LPUSH writes seqs at decreasing
positions, RPUSH at increasing). Non-UTF-8 item bytes round-trip via
the typed {"base64":"..."} envelope, sharing marshalRedisBinaryValue
with the hash encoder. TTL records on !redis|ttl| route into the list
JSON's expire_at_ms field via the HandleTTL switch (mirrors the hash
fold-into-record policy; lists carry no separate sidecar).
Refactor: extract flushWideColumnDir generic helper into redis_string.go.
Both flushHashes and flushLists now call it, eliminating ~30 lines of
"sort user keys + iterate" duplication that dupl would otherwise flag
on every new wide-column encoder. flushHashes diff is the inline
substitution; behaviour is unchanged.
Tests cover: LPUSH ordering preserved across out-of-order
HandleListItem submissions; empty list still emits a file (LLEN==0 is
observable); TTL inlined from scan index; length-mismatch warning
shape; binary item base64 envelope; meta-delta key silently skipped;
claim key silently skipped; malformed meta length rejected;
truncated item key rejected.
Phase 0a remaining after this PR: redis_set / redis_zset /
redis_stream encoders, the cmd/elastickv-snapshot-decode CLI, the
cmd/elastickv-snap-token helper, and the docs/operations runbook.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdds a Redis list backup encoder producing per-list JSON files, inlines list TTLs, introduces a generic wide-column directory flush helper shared by hashes and lists, refactors hash flushing to use it, and adds tests including a hash-meta overflow regression. ChangesRedis list backup encoder with wide-column flushing
Sequence Diagram(s)sequenceDiagram
participant Snapshot
participant RedisDB
participant Handler (ListMeta/ListItem)
participant listState
participant flushWideColumnDir
participant writeListJSON
Snapshot->>RedisDB: stream records (meta/item/TTL)
RedisDB->>Handler: dispatch to HandleListMeta or HandleListItem
Handler->>listState: create/update per-list buffer (declaredLen, items, TTL)
RedisDB->>flushWideColumnDir: finalize lists -> iterate keys
flushWideColumnDir->>writeListJSON: per-key callback to marshal and write JSON
writeListJSON-->>Snapshot: atomic lists/<encoded>.json written
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
|
@claude review |
|
Claude finished @bootjp's task in 5m 45s —— View job Code Review
Overall this is a well-structured PR. The list encoder faithfully mirrors the hash encoder's patterns, the key format aligns correctly with the live store, and the JSON shape matches the design doc ( Findings1. Inconsistent meta-value overflow handling between hash and list encoders (Minor)
st.declaredLen = int64(binary.BigEndian.Uint64(value)) //nolint:gosec // signed int64 by design
if rawLen > math.MaxInt64 {
return cockroachdberr.Wrapf(ErrRedisInvalidListMeta, "declared len %d overflows int64", rawLen)
}The list encoder is more defensive (good), but the inconsistency is worth backporting. With the hash encoder, a live store that somehow stores a field count with the high bit set would produce a negative 2.
|
There was a problem hiding this comment.
Code Review
This pull request introduces a Redis list encoder to the backup system. It includes the implementation of HandleListMeta, HandleListItem, and flushLists to process and store Redis list data as JSON. Additionally, it refactors the existing hash flushing logic into a generic flushWideColumnDir helper to be shared across wide-column types. Comprehensive tests are added to verify the list encoding logic, including TTL handling and error cases. I have no feedback to provide as there were no review comments to assess.
…rage Claude bot round-1 review surfaced two follow-ups worth backporting from the list encoder to the hash encoder for cross-encoder symmetry, plus one coverage gap and one design-intent comment. 1. HandleHashMeta now rejects uint64 declared-field counts with the high bit set instead of silently wrapping them to a negative int64 declaredLen. Without the guard, a corrupted store would fire spurious `redis_hash_length_mismatch` warnings on every flush regardless of how many fields actually arrived. Mirrors the list encoder's symmetric guard so both wide-column encoders fail closed on the same shape of corruption. Caller audit: HandleHashMeta has zero production callers in the repo (the decoder driver pipeline is not yet in tree); all test callers feed encodeHashMetaValue(N) with non-negative small ints, so none trip the new check. The new error case wraps the same ErrRedisInvalidHashMeta sentinel the existing length-check returns, so caller-side error-classification stays unchanged — purely a fail-closed expansion of an existing error class. 2. New regression test TestRedisDB_HashRejectsOverflowingMetaValue pins the new guard with a 1<<63 input. 3. New regression test TestRedisDB_ListItemsWithoutMetaStillEmitsFile closes the round-1 coverage gap: !lst|itm| records may arrive without a paired !lst|meta| in dump order, and the encoder must still emit the per-list JSON without firing the length-mismatch warning (metaSeen=false means no "declared" baseline to compare against). 4. Documented flushWideColumnDir's fail-fast-per-type error policy. The earlier "continue past per-key flush errors" alternative trades a noisy hard failure for a quiet soft failure that survives `find -name '*.json'` scrutiny — calling that out so future contributors don't "fix" the early return to a continue. go test -race ./internal/backup, golangci-lint, and go vet all clean.
|
@claude review Round 2 — claude bot の round-1 レビュー4件のうち2件 (#1, #3) を対応: #1 Hash encoder overflow guard backport — Caller audit: #3 items-without-meta regression test — 加えて:
スキップ:
|
|
Claude finished @bootjp's task in 1m 34s —— View job Code Review (Round 2)
Round 2 is clean. All four cited findings are addressed correctly and the implementations are sound. Finding #1 — Hash overflow guard (
|
Summary
!lst|meta|+!lst|itm|→lists/<key>.jsonencoder per the Phase 0 design doc (docs/design/2026_04_29_proposed_snapshot_logical_decoder.md).!redis|ttl|for a list user key fold into the list JSON'sexpire_at_msfield via theHandleTTLswitch — same policy as the hash encoder (no separate sidecar).!lst|meta|d|...deltas and!lst|claim|...POP tombstones are silently skipped:!lst|itm|keys are the source of truth at backup time, and the live read path (fetchListRangeinadapter/redis.go:3281) doesn't consult claims either — POPs delete the item key in the same OCC commit.flushWideColumnDirgeneric helper intoredis_string.go. BothflushHashesandflushListsnow call it, eliminating the ~30-line "create subdir + sort user keys + iterate" duplication that would otherwise be repeated for every wide-column encoder.Test plan
go test -race ./internal/backup/...(all 5 list tests + existing hash/string suite pass, 1.3s)golangci-lint run ./internal/backup/...(0 issues)HandleListItemsubmissions; empty list still emits a file (LLEN==0 observable); TTL inlined from scan index; length-mismatch warning shape; binary item base64 envelope; meta-delta key silently skipped; claim key silently skipped; malformed meta length rejected; truncated item key rejected.Self-review (5 lenses)
writeFileAtomic(tmp+rename). No write path drops records — only!lst|meta|d|/!lst|claim|records are skipped, and both are non-source-of-truth in the live read path.RedisDB.Handle*methods are documented not goroutine-safe; the decoder pipeline is sequential per scope. No new locks or shared state.map[int64][]bytekeyed by seq. Single allocation per item viabytes.Clone. Sort at flush is O(n log n) per list; lists are bounded bymaxWideColumnItemson the live side.declared_lenmismatch warns viaredis_list_length_mismatch; ascending-seq order is deterministic across runs (Go's randomised map iteration is mitigated by the sort).Phase 0a remaining after this PR
redis_set.go/redis_zset.go/redis_stream.goencoderscmd/elastickv-snapshot-decode/CLI binarycmd/elastickv-snap-tokenhelperdocs/operations/snapshot_restore.mdrunbookSummary by CodeRabbit