feat: support parsing rdb preamble in aof loader#1042
Conversation
suxb201
left a comment
There was a problem hiding this comment.
Thanks for tackling this — single-file AOF with RDB preamble is a real gap. The approach of refactoring ParseRDB into a stream-based ParseRDBStream so the same bufio.Reader can carry across the RDB→AOF boundary is the right design. A few things to address before merging:
Must fix
1. Operator precedence bug in aof.go
isRDB := (err == nil) && (n >= 5 && bytes.Equal(sig[:5], []byte("REDIS"))) || (n >= 6 && bytes.Equal(sig[:6], []byte("VALKEY")))&& binds tighter than ||, so this parses as ((err==nil) && REDIS-check) || VALKEY-check — the VALKEY branch no longer requires err == nil. Compare with the existing (correct) version in parsing_aof.go:
isRDB := (err == nil) && ((n >= 5 && ...REDIS) || (n >= 6 && ...VALKEY))Please match it.
2. Don't swallow the checksum read error in rdb.go
case kEOF:
io.ReadFull(rd, make([]byte, 8)) // read and ignore checksum
returnFor a pure RDB file this is harmless, but for the AOF-with-RDB-preamble case (the whole point of this PR), a truncated CRC means subsequent AOF parsing will be misaligned and you'll get a confusing "Bad File format" panic far from the root cause. At minimum, panic/return on error here.
Strongly suggest
3. useRDBPreamble int should be bool — int with values 0/1 isn't idiomatic Go, and the field's name already conveys a boolean concept.
4. Consider dropping the useRDBPreamble flag entirely. LoadSingleAppendOnlyFile can always do the 6-byte magic check — it's cheap, and it makes the loader strictly more capable without requiring callers to remember to opt in. The current state where one caller sets AOFUseRDBPreamble = 1 and another leaves it at 0 is fragile.
5. The if updateFunc != nil guard in NewLoader is a no-op — ld.updateFunc = updateFunc has identical behavior since the field is already nil. Please remove.
6. NewLoader signature now takes (name, useRDBPreamble, filePath, ch) — two adjacent strings make argument swaps easy. If #4 is adopted, this resolves itself.
Nits
path.Base→filepath.Basefor local file paths.- The RDB magic check is now duplicated between
aof.goandparsing_aof.go— worth a small helper, e.g.looksLikeRDBPreamble(*bufio.Reader)usingPeekinstead ofRead+Seek. - The comment `// Skipped RDB checksum and has not been processed yet.` was carried over from the existing path but contradicts what this PR actually does (it does process the checksum now). Please update.
- The return value of
ParseRDBStream(replStreamDbId) is discarded. In practice this is fine because real AOF tails contain explicitSELECTcommands, but a one-line comment noting that would help future readers.
Testing
Could you add a test? A synthetic input that's REDIS<version> ... <EOF byte> <8-byte CRC> *<RESP commands> would catch both the alignment fix and any future regression in the handover. Bonus points for a VALKEY-magic case.
Problem:
The loader is unable to parse single-file AOFs with RDB preamble (pre-7.0 Redis), leading to errors or data loss of the incremental logs.
Solution:
Result:
Supports loading and processing pre-7.0 mixed-mode AOF files.