Repository navigation
Conversation
|
Hi @httpanish, Thank you for your pull request. Please review our Contributing Guide. Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed. |
|
hi @jihuayu @nagisa-kunhah! could you take a look at my PR when there's time please? |
nagisa-kunhah
left a comment
nagisa-kunhah
left a comment
There was a problem hiding this comment.
Totally LGTM. I left two non-blocking issue here, and you can choose whether to fix it or not. Thanks for your contribution!
|
|
||
| TEST_F(RedisCuckooFilterTest, AddNxBasic) { |
There was a problem hiding this comment.
Could we add a test that triggers expansion via AddNx and verifies that duplicate items are still detected afterward?
| auto s = getCuckooChainMetadata(ctx, ns_key, &metadata); | ||
| bool is_new = false; | ||
| if (s.IsNotFound()) { | ||
| metadata = CuckooChainMetadata(); | ||
| metadata.size = 0; | ||
| metadata.base_capacity = kCFDefaultCapacity; | ||
| metadata.bucket_size = kCFDefaultBucketSize; | ||
| metadata.max_iterations = kCFDefaultMaxIterations; | ||
| metadata.expansion = kCFDefaultExpansion; | ||
| metadata.n_filters = 1; | ||
| metadata.num_deleted_items = 0; | ||
| metadata.page_size = kCuckooFilterDefaultPageSize; | ||
| is_new = true; | ||
| } else if (!s.ok()) { | ||
| return s; | ||
| } |
There was a problem hiding this comment.
Since this logic overlaps with the logic in the Add command, can we extract it into a common function for easier future maintenance?
Links to #3552
This PR adds the CF.ADDNX command
CF.ADDNX key itemCuckoo:Chain::AddNx: checks sub-fillers from oldest to newest unlike MExists so newest filter's pages which the insert tries first are still cached and aren't read from RocksDB twiceI have used Claude Opus 5.5 for generating integration and unit tests