search

valkey-io / valkey-bloom

Rust based Valkey Module which provides a BloomFilter data type / APIs
10 stars 9 forks source link

add bf.load to support bloom filter aofrewrite. #17

Closed wuranxx closed 1 week ago

wuranxx commented 1 month ago

this pr is for #8 https://github.com/valkey-io/valkey-bloom/issues/8.

design proposal

I use serde and bincode to deserialize and deserialize BloomFilterType. add data version in first element.

bf.load not support override old key.

wuranxx commented 1 month ago

@KarthikSubbarao Please help review this PR.

wuranxx commented 1 month ago

@KarthikSubbarao The review comments have been modified and replied. Please review them again.

wuranxx commented 4 weeks ago

@KarthikSubbarao I think another purpose of BF.DUMP is for migration scenarios. When tools like redisshake perform data migration between two Redis instances, the valkey-bloom type keys can be replayed using BF.LOAD and BF.DUMP.

If you believe this command is ultimately unnecessary, please let me know, and I will remove it. (About BF.DUMP review can't reply, so i add a new comment to reply it.)

wuranxx commented 3 weeks ago

@KarthikSubbarao The review comments have been modified and replied. Please review them again, thank you.

zackcam commented 2 weeks ago

Quick note: When you get the latest changes your changes will cause the tests in test_bloom_metrics.py to fail, due to the new size of BloomFilterType, to fix this you can update this file and line to be DEFAULT_BLOOM_FILTER_SIZE = 179961.

wuranxx commented 2 weeks ago

Quick note: When you get the latest changes your changes will cause the tests in test_bloom_metrics.py to fail, due to the new size of BloomFilterType, to fix this you can update this file and line to be DEFAULT_BLOOM_FILTER_SIZE = 179961.

I've removed the version field from BloomFitlerType.

KarthikSubbarao commented 1 week ago

The PR is almost ready to be merged in. Just have few comments on fixing the validation logic of bloom filters restored from BF.LOAD. We can merge it in after this

KarthikSubbarao commented 1 week ago

Thank you, @wuranxx . Can you update the PR Description? It currently references BF.DUMP. We can update it to reference BF.LOAD and I will merge the PR after that

wuranxx commented 1 week ago

Thank you, @wuranxx . Can you update the PR Description? It currently references BF.DUMP. We can update it to reference BF.LOAD and I will merge the PR after that

updated.

KarthikSubbarao commented 1 week ago

Thank you for helping with AOF Rewrite support, @wuranxx !