Skip to content

refactor(cuckoo-filter): move page cache ownership to chain - #3609

Open
nagisa-kunhah wants to merge 7 commits into
apache:unstablefrom
nagisa-kunhah:rfc/page-cache-into-chain
Open

nagisa-kunhah wants to merge 7 commits into
apache:unstablefrom
nagisa-kunhah:rfc/page-cache-into-chain

Conversation

@nagisa-kunhah

@nagisa-kunhah nagisa-kunhah commented Sep 6, 2026 •

Copy link
Copy Markdown
Contributor
  • Move cuckoo filter page-cache ownership from individual subfilters to the chain operation.
  • Share cached pages across subfilters and commit dirty pages with metadata at the chain layer.
  • Add slot-level undo for failed kick-out attempts while preserving previously staged page mutations.
  • Separate the insert, kick-out, and expansion stages from persistence so future multi-item insert operations can reuse the same metadata and page cache before committing once.

Testing

  • ./x.py check format
  • cmake --build build --target kvrocks -j2
  • cmake --build build --target unittest -j2
  • ./build/unittest --gtest_filter='*Cuckoo*'
  • ./build/unittest
    This PR was written using codex and GPT-5.6 sol

@nagisa-kunhah
nagisa-kunhah marked this pull request as ready for review September 8, 2026 18:03
@jihuayu
jihuayu requested a balanced review from Copilot September 11, 2026 01:56

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The shared cache can retain an unbounded number of clean pages while scanning long filter chains.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Moves cuckoo-filter page-cache ownership to the chain layer, enabling shared staging and atomic persistence.

Changes:

  • Shares cached pages across subfilters.
  • Adds slot-level rollback for failed kick-outs.
  • Separates insertion, expansion, and persistence stages.
File summaries
File Description
src/types/cuckoo_filter_page.h Defines undo metadata and cache APIs.
src/types/cuckoo_filter_page.cc Implements slot mutation rollback.
src/types/cuckoo_filter_sub_filter.h Makes subfilters borrow the shared cache.
src/types/cuckoo_filter_sub_filter.cc Preserves staged mutations during rollback.
src/types/redis_cuckoo_chain.h Refactors chain insertion interfaces.
src/types/redis_cuckoo_chain.cc Centralizes cache ownership and persistence.
tests/cppunit/types/cuckoo_filter_test.cc Tests rollback and shared-cache behavior.
Review details
  • Files reviewed: 7/7 changed files
  • Comments generated: 1
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/types/redis_cuckoo_chain.cc
@jihuayu
jihuayu requested review from jihuayu and a balanced review from Copilot September 27, 2026 09:50

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🟢 Approval recommended

The ownership refactor preserves atomic persistence and includes focused coverage for rollback and cache eviction.

Review effort: Balanced
Findings: None

Resolved since last review (1)

@nagisa-kunhah

nagisa-kunhah commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor Author

Hi @jihuayu I have a minor question about my PRs: Since this PR is related to the cuckoo-filter framework, should we complete this PR first and then process the subsequent PRs, such as #3467 and #3567? I also want to know why #3467 didn't pass CI before. Should we re-trigger CI for #3467 ?

@jihuayu

jihuayu commented Sep 27, 2026

Copy link
Copy Markdown
Member

@nagisa-kunhah If #3467 and #3567 don't depend on the code in this PR, then it's not an issue.

I just forgot about PR #3467. It looks like the issue was caused by a GitHub service problem. I've already re-triggered it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants