Package review PR #1

KarthikSubbarao · 2024-04-29T19:40:11Z

This is a PR which merges current changes in unstable into an empty branch in order to help with a review code of the entire codebase.

src/commands/bloom.rs

hpatro · 2024-05-01T22:03:26Z

src/commands/bloom.rs

+    let mut result = Vec::new();
+    match value {
+        Some(bf) => {
+            for item in input_args.iter().take(argc).skip(idx) {
+                result.push(RedisValue::Integer(bf.add_item(item.as_slice())));
+            }
+            Ok(RedisValue::Array(result))
+        }
+        None => {
+            if nocreate {
+                return Err(RedisError::Str("ERR not found"));
+            }
+            let mut bf = BloomFilterType::new_reserved(fp_rate, capacity, expansion);
+            for item in input_args.iter().take(argc).skip(idx) {
+                result.push(RedisValue::Integer(bf.add_item(item.as_slice())));
+            }
+            match filter_key.set_value(&BLOOM_FILTER_TYPE, bf) {
+                Ok(_) => Ok(RedisValue::Array(result)),
+                Err(_) => Err(RedisError::Str(ERROR)),
+            }
+        }


Could we check the bloom filter exists or not first and then insert the data in a single flow. Don't like the code duplication around insertion.

I can create a separate function for multi adds and call it from both flows. I wanted to handle both through the same flow, however it will be out of reference scope and result in a moved error

hpatro · 2024-05-01T22:08:06Z

src/commands/bloom_util.rs

+pub struct BloomFilterType {
+    pub expansion: u32,
+    pub fp_rate: f32,
+    pub filters: Vec<BloomFilter>,


Does the underlying support scalable bloom filters or do we need to support this mechanism?

This particular library does not have auto scaling

https://docs.rs/bloomfilter/1.0.13/bloomfilter/struct.Bloom.html

src/commands/bloom_util.rs

hpatro · 2024-05-01T22:12:18Z

src/commands/bloom_util.rs

+                let new_capacity = filter.capacity * self.expansion;
+                let mut new_filter = BloomFilter::new(self.fp_rate, new_capacity);
+                // Add item.
+                new_filter.bloom.set(item);
+                new_filter.num_items += 1;
+                self.filters.push(new_filter);


Shouldn't we fail the request if the expansion is much higher than we can support?

expansion really means expansion_rate. I can rename this variable if that would help clarify this.

I don't think there is a limit on the number of sub filters an object can have. But (if we want) we can define a config for this to either silently fail expansion (by allowing a set) beyond a limit of X sub filters per object OR explicitly return an error (without a set).

One other aspect we should handle this checking / rejecting based on memory overhead of every operation that creates a new BloomFilter object (BF.ADD, BF.MADD, BD.RESERVE, BF.INSERT, RDB Load). Before any of these operations, we should probably check the memory usage and reject the operations if there is not sufficient space. We have a mechanism to compute the estimated additional memory overhead per creation.

src/wrapper/bloom_callback.rs

src/commands/bloom_data_type.rs

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

… objects. Update RDB load/save Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

* Support for DEBUG DIGEST module data type callback Signed-off-by: Nihal Mehta <nnmehta@amazon.com> * Update test cases Signed-off-by: Nihal Mehta <nnmehta@amazon.com> * Move digest to wrapper Signed-off-by: Nihal Mehta <nnmehta@amazon.com> * Update tests Signed-off-by: Nihal Mehta <nnmehta@amazon.com> * Add more scenarios for debug test Signed-off-by: Nihal Mehta <nnmehta@amazon.com> * Clean code and add scenario for debug test Signed-off-by: Nihal Mehta <nnmehta@amazon.com> --------- Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

…filter dependency to version 3.0 (#23) * Updating how we create BloomFilter from rdb loads. BloomFilter vec now has capacity of filter we are loading from Signed-off-by: zackcam <zackcam@amazon.com> * Updating bloomfilter dependency to version 3, fixing breaking changes as well Signed-off-by: zackcam <zackcam@amazon.com> * Updating the digest changes to follow updated version of bloom. As well as removing unnecesary fields saved in rdb Signed-off-by: zackcam <zackcam@amazon.com> * Update log in src/bloom/data_type.rs Signed-off-by: KarthikSubbarao <karthikrs2021@gmail.com> * Update comment in src/bloom/utils.rs Signed-off-by: KarthikSubbarao <karthikrs2021@gmail.com> * Clippy error in src/bloom/data_type.rs Signed-off-by: KarthikSubbarao <karthikrs2021@gmail.com> --------- Signed-off-by: zackcam <zackcam@amazon.com> Signed-off-by: KarthikSubbarao <karthikrs2021@gmail.com> Co-authored-by: KarthikSubbarao <karthikrs2021@gmail.com>

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

…n a non bloom key (#30) Signed-off-by: zackcam <zackcam@amazon.com>

…r structs (#24) * Updating defragmentation to defrag both bloomfiltertype and bloomfiler structs Signed-off-by: zackcam <zackcam@amazon.com> * Draft: Extra debugging and more factors being defragged Signed-off-by: zackcam <zackcam@amazon.com> * Updating defrag and tests to use cursors and make test more robust by getting hits Signed-off-by: zackcam <zackcam@amazon.com> * Fixing merge conflicts Signed-off-by: zackcam <zackcam@amazon.com> * Fixing merge conflicts with random seed. Updating defrag test to use mexists and add_items_till_capacity. Refactored metric tracking as well to reduce code repetition Signed-off-by: zackcam <zackcam@amazon.com> --------- Signed-off-by: zackcam <zackcam@amazon.com>

…commands contain the correct acl categories (#29) Signed-off-by: zackcam <zackcam@amazon.com>

Signed-off-by: VanessaTang <yuetan@amazon.com>

…e properties on replica nodes to match the object on the primary (#32) Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

…s get incremented in defrag test (#33) Signed-off-by: zackcam <zackcam@amazon.com>

…fail (#34) * Deflaking tests to remove chance of false positives causing tests to fail Signed-off-by: zackcam <zackcam@amazon.com> * Update tests/test_bloom_command.py Co-authored-by: KarthikSubbarao <karthikrs2021@gmail.com> Signed-off-by: zackcam <zackcam@amazon.com> --------- Signed-off-by: zackcam <zackcam@amazon.com> Co-authored-by: KarthikSubbarao <karthikrs2021@gmail.com>

Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Signed-off-by: VanessaTang <yuetan@amazon.com>

#37) Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Signed-off-by: KarthikSubbarao <karthikrs2021@gmail.com>

Signed-off-by: zackcam <zackcam@amazon.com>

Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

…om filter can reach the desired size (#41) * Adding optional arg to BF.INSERT to allow users to check if their bloom filter can reach the desired size Signed-off-by: zackcam <zackcam@amazon.com> * Fixing ATLEASTCAPACITY calculation as well as adding MAXCAPACITY functionality for info Signed-off-by: zackcam <zackcam@amazon.com> --------- Signed-off-by: zackcam <zackcam@amazon.com>

…ity caluclation matches actual capacity (#43) Signed-off-by: zackcam <zackcam@amazon.com>

…44) file as it can be taken from a dependency and moving metric increments to after creates Signed-off-by: zackcam <zackcam@amazon.com>

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch 3 times, most recently from 61aa1fb to 0e4d4d1 Compare April 30, 2024 18:14

KarthikSubbarao changed the title ~~Package review~~ Package review PR May 1, 2024

KarthikSubbarao self-assigned this May 1, 2024

hpatro reviewed May 1, 2024

View reviewed changes

KarthikSubbarao force-pushed the unstable branch from 055728d to 7027032 Compare July 16, 2024 03:25

KarthikSubbarao force-pushed the unstable branch from c87fd97 to 869a254 Compare August 23, 2024 17:20

KarthikSubbarao pushed a commit that referenced this pull request Aug 23, 2024

Add information about RFC process (#1)

657f83c

Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Madelyn Olson <madelyneolson@gmail.com>

KarthikSubbarao added 21 commits September 10, 2024 23:06

Initial commit

5237a10

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add support for BF.ADD, BF.EXISTS, BF.CARD

a96c08a

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Update README

be97a25

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Optimize bloom operations to use a reference to already created Bloom…

5ebc1cb

… objects. Update RDB load/save Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Remove older Bloom APIs using prev serialization apporach

8557b55

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add Dev Profile in Cargo.toml for debugging

1729520

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Create Module Datatype and support RDB load, save, free

be80d9d

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Update README.md

ed4d0f9

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add support for BF.RESERVE and expansion config

2db0e4b

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add support for BF.INFO

778c219

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add support for BF.MEXISTS

b11970e

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add support for BF.MADD

bdf5f0d

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Update error handling / messages and update expansion logic

8f5e3fc

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add auto scaling support for bloom filters

5545680

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Fix RDB Save for scaled filters

42d015c

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Refactoring

481ecac

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Update TODOs

ff60817

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Fix mem_usage calculation

1c1b6ab

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Update Cargo.toml

d00fa99

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

minor refactoring

053d5e9

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

Add support for BF.INSERT and fix multi add logic

fbdfb5b

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch from 3f24e34 to 7c25468 Compare November 30, 2024 07:25

zackcam and others added 2 commits December 3, 2024 22:02

Support random seed per bloom object by default (configurable) (#27)

1b0d819

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch from 99c6f58 to 1b0d819 Compare December 5, 2024 08:06

nnmehta and others added 6 commits December 6, 2024 13:29

Add tightening_ratio support in BloomFilterType structure (#26)

f10a4e5

Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

Adding a wrong type error message when performing a bloom operation o…

eaa218f

…n a non bloom key (#30) Signed-off-by: zackcam <zackcam@amazon.com>

Adding bloom acl category to commands. Added tests to check that the …

a5ec8de

…commands contain the correct acl categories (#29) Signed-off-by: zackcam <zackcam@amazon.com>

Update default configuration and related test (#28)

4a8af80

Signed-off-by: VanessaTang <yuetan@amazon.com>

Deterministic replication - bloom object creation should have the sam…

8b94c17

…e properties on replica nodes to match the object on the primary (#32) Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch from 391d947 to 8b94c17 Compare December 12, 2024 21:35

zackcam and others added 4 commits December 12, 2024 18:30

Adding bloom defrag hits and misses to info, added check these metric…

3589d30

…s get incremented in defrag test (#33) Signed-off-by: zackcam <zackcam@amazon.com>

Add TIGHTENING Ratio and FP_Rate configs support (#31)

9baf6fc

Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

Fix size check and update build script to fail on warnings

6a514bf

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch from fcaad77 to 6a514bf Compare December 19, 2024 18:27

YueTang-Vanessa and others added 2 commits January 2, 2025 19:22

Add test for loading rdb onto non-bloom cluster (#36)

1c949fb

Signed-off-by: VanessaTang <yuetan@amazon.com>

Large bloom object handling + Rename to BloomFilterType to BloomObject (

ab158bb

#37) Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch from 9859dd6 to ab158bb Compare January 11, 2025 00:04

ACL Updates, BF.INSERT updates, other misc updates (#38)

a9b578e

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch from d3d7522 to a9b578e Compare January 12, 2025 02:55

KarthikSubbarao and others added 7 commits January 11, 2025 20:01

Update README.md (#39)

1bb7d25

Signed-off-by: KarthikSubbarao <karthikrs2021@gmail.com>

Adding a bloom command keyspace test (#40)

591ab10

Signed-off-by: zackcam <zackcam@amazon.com>

Add dump and restore test (#42)

20efd95

Signed-off-by: Nihal Mehta <nnmehta@amazon.com>

Fixing capacity calcualtion and adding a unit test to make sure capac…

56f631f

…ity caluclation matches actual capacity (#43) Signed-off-by: zackcam <zackcam@amazon.com>

Code cleanup: removing unused dependencies in tests, removing digest (#…

8077d58

…44) file as it can be taken from a dependency and moving metric increments to after creates Signed-off-by: zackcam <zackcam@amazon.com>

Add License file following Valkey (#46)

8f1c9f4

Signed-off-by: Karthik Subbarao <karthikrs2021@gmail.com>

KarthikSubbarao force-pushed the unstable branch from f46179b to 8f1c9f4 Compare February 15, 2025 02:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Package review PR #1

Package review PR #1

KarthikSubbarao commented Apr 29, 2024 •

edited

Loading

hpatro May 1, 2024

KarthikSubbarao May 1, 2024

hpatro May 1, 2024

KarthikSubbarao May 1, 2024

hpatro May 1, 2024

KarthikSubbarao May 1, 2024

KarthikSubbarao May 2, 2024

Package review PR #1

Are you sure you want to change the base?

Package review PR #1

Conversation

KarthikSubbarao commented Apr 29, 2024 • edited Loading

hpatro May 1, 2024

Choose a reason for hiding this comment

KarthikSubbarao May 1, 2024

Choose a reason for hiding this comment

hpatro May 1, 2024

Choose a reason for hiding this comment

KarthikSubbarao May 1, 2024

Choose a reason for hiding this comment

hpatro May 1, 2024

Choose a reason for hiding this comment

KarthikSubbarao May 1, 2024

Choose a reason for hiding this comment

KarthikSubbarao May 2, 2024

Choose a reason for hiding this comment

KarthikSubbarao commented Apr 29, 2024 •

edited

Loading