Conversation
Code Review by Qodo
1. RDB save crashes on cuckoo
|
| case CUCKOO_FILTER_TAG: | ||
| return OBJ_CUCKOOFILTER; |
There was a problem hiding this comment.
1. Rdb save crashes on cuckoo 🐞 Bug ≡ Correctness
CompactObj::ObjType() can now return OBJ_CUCKOOFILTER, but RdbObjectType() in rdb_save.cc has no case for it and falls through to LOG(FATAL), crashing during snapshot/backup when such a value is saved.
Agent Prompt
## Issue description
`CompactObj` can now represent `OBJ_CUCKOOFILTER` (via `CUCKOO_FILTER_TAG`), but the RDB serialization dispatch (`RdbObjectType`/`SaveObject` and the loader side) does not recognize this type. If a cuckoo-filter value is present during snapshot/backup, `RdbObjectType()` will hit its fallback `LOG(FATAL)` path and terminate the process.
## Issue Context
Dragonfly uses custom DF RDB types for module-like objects (JSON/SBF/CMS/TOPK). CuckooFilter needs the same end-to-end wiring: type id, save path, and load path.
## Fix Focus Areas
- src/server/rdb_extensions.h[11-25]
- src/server/rdb_save.cc[165-212]
- src/server/rdb_save.cc[334-376]
- src/server/rdb_load.cc[1261-1294]
- src/core/compact_object.cc[911-952]
## What to implement
1. Introduce a new DF RDB type constant (e.g. `RDB_TYPE_CUCKOOFILTER = 37`) and include it in `rdbIsObjectTypeDF`.
2. Update `RdbObjectType(const CompactObj&)` to map `OBJ_CUCKOOFILTER` to `RDB_TYPE_CUCKOOFILTER`.
3. Add `SaveCuckooFilterObject(const PrimeValue&)` and dispatch it from `SaveObject()` when `obj_type == OBJ_CUCKOOFILTER`.
4. Update `RdbLoaderBase` switch to recognize `RDB_TYPE_CUCKOOFILTER` and implement `ReadCuckooFilter()` that reconstructs `PrimeValue` via `SetCuckooFilter(...)` and restores the filter contents.
5. Add a save+load roundtrip test similar to SBF/CMS/TOPK to ensure snapshots work with cuckoo filters.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
🤖 Augment PR SummarySummary: This PR adds Cuckoo Filter support to Dragonfly’s Changes:
🤖 Was this summary useful? React with 👍 or 👎 |
fe039aa to
c13082f
Compare
Signed-off-by: Kostas Kyrimis <kostas@dragonflydb.io>
| [](const SubFilter& sf) { return sf.size(); }); | ||
| size_t CuckooFilter::MallocUsed() const { | ||
| size_t res = sizeof(CuckooFilter) + filters_.capacity() * sizeof(SubFilter); | ||
| for (const SubFilter& sf : filters_) { |
There was a problem hiding this comment.
@dranikpg never again, I accept my defeat 😄
|
augment review |
| void SetCMS(uint32_t width, uint32_t depth); | ||
| CMS* GetCMS() const; | ||
|
|
||
| void SetCuckooFilter(CuckooFilter* cf) { |
There was a problem hiding this comment.
SetCuckooFilter(CuckooFilter* cf) stores a raw pointer, but CompactObj::Free() later calls DeleteMR<CuckooFilter>, which assumes cf was allocated via CompactObj::AllocateMR (same thread-local MR). Without an explicit ownership/allocator contract here, it’s easy for a future caller to pass a differently-allocated filter and trigger invalid deallocation.
Severity: medium
🤖 Was this useful? React with 👍 or 👎, or 🚀 if it prevented an incident/outage.
integrate CuckooFilter to CompactObject
resolves #7579