-
Notifications
You must be signed in to change notification settings - Fork 10
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for DEBUG DIGEST module data type callback #21
Changes from 4 commits
4edc19c
743516d
988f056
cba8f17
0232f2d
dc303c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
use std::os::raw::c_char; | ||
use valkey_module::raw; | ||
use valkey_module::ValkeyString; | ||
|
||
/// `Digest` is a high-level rust interface to the Valkey module C API | ||
/// abstracting away the raw C ffi calls. | ||
pub struct Digest { | ||
pub dig: *mut raw::RedisModuleDigest, | ||
} | ||
|
||
impl Digest { | ||
pub const fn new(dig: *mut raw::RedisModuleDigest) -> Self { | ||
Self { dig } | ||
} | ||
|
||
/// Returns the key name of this [`Digest`]. | ||
/// | ||
/// # Panics | ||
/// | ||
/// Will panic if `RedisModule_GetKeyNameFromDigest` is missing in redismodule.h | ||
pub fn get_key_name(&self) -> ValkeyString { | ||
ValkeyString::from_redis_module_string(std::ptr::null_mut(), unsafe { | ||
raw::RedisModule_GetKeyNameFromDigest | ||
.expect("RedisModule_GetKeyNameFromDigest is not available.")(self.dig) | ||
.cast_mut() | ||
}) | ||
} | ||
|
||
/// Returns the database ID of this [`Digest`]. | ||
/// | ||
/// # Panics | ||
/// | ||
/// Will panic if `RedisModule_GetDbIdFromDigest` is missing in redismodule.h | ||
pub fn get_db_id(&self) -> i32 { | ||
unsafe { | ||
raw::RedisModule_GetDbIdFromDigest | ||
.expect("RedisModule_GetDbIdFromDigest is not available.")(self.dig) | ||
} | ||
} | ||
|
||
/// Adds a new element to this [`Digest`]. | ||
/// | ||
/// # Panics | ||
/// | ||
/// Will panic if `RedisModule_DigestAddStringBuffer` is missing in redismodule.h | ||
pub fn add_string_buffer(&mut self, ele: &[u8]) { | ||
unsafe { | ||
raw::RedisModule_DigestAddStringBuffer | ||
.expect("RedisModule_DigestAddStringBuffer is not available.")( | ||
self.dig, | ||
ele.as_ptr().cast::<c_char>(), | ||
ele.len(), | ||
) | ||
} | ||
} | ||
|
||
/// Similar to [`Digest::add_string_buffer`], but takes [`i64`]. | ||
/// | ||
/// # Panics | ||
/// | ||
/// Will panic if `RedisModule_DigestAddLongLong` is missing in redismodule.h | ||
pub fn add_long_long(&mut self, ll: i64) { | ||
unsafe { | ||
raw::RedisModule_DigestAddLongLong | ||
.expect("RedisModule_DigestAddLongLong is not available.")(self.dig, ll) | ||
} | ||
} | ||
|
||
/// Ends the current sequence in this [`Digest`]. | ||
/// | ||
/// # Panics | ||
/// | ||
/// Will panic if `RedisModule_DigestEndSequence` is missing in redismodule.h | ||
pub fn end_sequence(&mut self) { | ||
unsafe { | ||
raw::RedisModule_DigestEndSequence | ||
.expect("RedisModule_DigestEndSequence is not available.")(self.dig) | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1 +1,2 @@ | ||
pub mod bloom_callback; | ||
pub mod digest; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,14 +20,23 @@ def test_basic_aofrewrite_and_restore(self): | |
bf_info_result_1 = client.execute_command('BF.INFO testSave') | ||
assert(len(bf_info_result_1)) != 0 | ||
curr_item_count_1 = client.info_obj().num_keys() | ||
|
||
# cmd debug digest | ||
cmd_debug = client.debug_digest() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
assert cmd_debug != None or 0000000000000000000000000000000000000000 | ||
debug_save = client.execute_command('DEBUG DIGEST-VALUE testSave') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
# save aof, restart sever | ||
client.bgrewriteaof() | ||
self.server.wait_for_action_done(ValkeyAction.AOF_REWRITE) | ||
# Keep the server running for 1 second more to have a larger uptime. | ||
time.sleep(1) | ||
self.server.restart(remove_rdb=False, remove_nodes_conf=False, connect_client=True) | ||
assert self.server.is_alive() | ||
debug_restart = client.debug_digest() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
assert debug_restart != None or 0000000000000000000000000000000000000000 | ||
debug_restore = client.execute_command('DEBUG DIGEST-VALUE testSave') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
assert debug_restart == cmd_debug | ||
assert debug_restore == debug_save | ||
|
||
# verify restore results | ||
curr_item_count_2 = client.info_obj().num_keys() | ||
|
@@ -49,12 +58,23 @@ def test_aofrewrite_bloomfilter_metrics(self): | |
for var in variables: | ||
self.client.execute_command(f'BF.ADD key1 {var}') | ||
|
||
# cmd debug digest | ||
cmd_debug = self.client.debug_digest() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
assert cmd_debug != None or 0000000000000000000000000000000000000000 | ||
debug_save = self.client.execute_command('DEBUG DIGEST-VALUE key1') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
|
||
# save aof, restart sever | ||
self.client.bgrewriteaof() | ||
self.server.wait_for_action_done(ValkeyAction.AOF_REWRITE) | ||
# restart server | ||
time.sleep(1) | ||
self.server.restart(remove_rdb=False, remove_nodes_conf=False, connect_client=True) | ||
assert self.server.is_alive() | ||
debug_restart = self.client.debug_digest() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
assert debug_restart != None or 0000000000000000000000000000000000000000 | ||
debug_restore = self.client.execute_command('DEBUG DIGEST-VALUE key1') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
assert debug_restart == cmd_debug | ||
assert debug_restore == debug_save | ||
|
||
# Check info for scaled bloomfilter matches metrics data for bloomfilter | ||
new_info_obj = self.client.execute_command(f'BF.INFO key1') | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -39,10 +39,19 @@ def test_copy_and_exists_cmd(self): | |
assert client.execute_command('EXISTS filter') == 1 | ||
mexists_result = client.execute_command('BF.MEXISTS filter item1 item2 item3 item4') | ||
assert len(madd_result) == 4 and len(mexists_result) == 4 | ||
# cmd debug digest | ||
cmd_debug = client.debug_digest() | ||
assert cmd_debug != None or 0000000000000000000000000000000000000000 | ||
debug_filter = client.execute_command('DEBUG DIGEST-VALUE filter') | ||
assert client.execute_command('COPY filter new_filter') == 1 | ||
debug_copy = client.debug_digest() | ||
assert debug_copy != None or 0000000000000000000000000000000000000000 | ||
debug_new_filter = client.execute_command('DEBUG DIGEST-VALUE filter') | ||
assert client.execute_command('EXISTS new_filter') == 1 | ||
copy_mexists_result = client.execute_command('BF.MEXISTS new_filter item1 item2 item3 item4') | ||
assert mexists_result == copy_mexists_result | ||
assert cmd_debug != debug_copy | ||
assert debug_new_filter == debug_filter | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1 . Can we move this into a new test called |
||
|
||
def test_memory_usage_cmd(self): | ||
client = self.server.get_new_client() | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,6 +17,9 @@ def test_non_scaling_filter(self): | |
error_count, add_operation_idx = self.add_items_till_capacity(client, filter_name, capacity, 1, item_prefix) | ||
with pytest.raises(Exception, match="non scaling filter is full"): | ||
client.execute_command(f'BF.ADD {filter_name} new_item') | ||
# cmd debug digest | ||
cmd_debug = client.debug_digest() | ||
assert cmd_debug != None or 0000000000000000000000000000000000000000 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove this |
||
# Validate that is is filled. | ||
info = client.execute_command(f'BF.INFO {filter_name}') | ||
it = iter(info) | ||
|
@@ -54,6 +57,8 @@ def test_non_scaling_filter(self): | |
item_prefix, | ||
) | ||
self.fp_assert(error_count, num_operations, expected_fp_rate, fp_margin) | ||
debug_copy = client.debug_digest() | ||
assert debug_copy != None or 0000000000000000000000000000000000000000 or cmd_debug | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the copied object's digest match? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove this |
||
# Validate correctness on a copy of a non scaling bloom filter. | ||
self.validate_copied_bloom_correctness(client, filter_name, item_prefix, add_operation_idx, expected_fp_rate, fp_margin, info_dict) | ||
|
||
|
@@ -67,7 +72,9 @@ def test_scaling_filter(self): | |
filter_name = "filter1" | ||
# Create a scaling bloom filter and validate its behavior. | ||
assert client.execute_command(f'BF.RESERVE {filter_name} {expected_fp_rate} {initial_capacity} EXPANSION {expansion}') == b"OK" | ||
|
||
# cmd debug digest | ||
cmd_debug = client.debug_digest() | ||
assert cmd_debug != None or 0000000000000000000000000000000000000000 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove this |
||
info = client.execute_command(f'BF.INFO {filter_name}') | ||
it = iter(info) | ||
info_dict = dict(zip(it, it)) | ||
|
@@ -127,5 +134,7 @@ def test_scaling_filter(self): | |
info = client.execute_command(f'BF.INFO {filter_name}') | ||
it = iter(info) | ||
info_dict = dict(zip(it, it)) | ||
debug_copy = client.debug_digest() | ||
assert debug_copy != None or 0000000000000000000000000000000000000000 or cmd_debug | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't the copied object's digest match? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can remove this |
||
# Validate correctness on a copy of a scaling bloom filter. | ||
self.validate_copied_bloom_correctness(client, filter_name, item_prefix, add_operation_idx, expected_fp_rate, fp_margin, info_dict) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -474,7 +474,7 @@ def port_tracker_fixture(self, resource_port_tracker): | |
self.port_tracker = resource_port_tracker | ||
|
||
def _get_valkey_args(self): | ||
self.args.update({"maxmemory":self.maxmemory, "maxmemory-policy":"allkeys-random", "activerehashing":"yes", "databases": self.num_dbs, "repl-diskless-sync": "yes", "save": ""}) | ||
self.args.update({"maxmemory":self.maxmemory, "maxmemory-policy":"allkeys-random", "activerehashing":"yes", "databases": self.num_dbs, "repl-diskless-sync": "yes", "save": "", "enable-debug-command":"yes"}) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let us also add testing in two other places:
|
||
self.args.update(self.get_custom_args()) | ||
return self.args | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the callback.
You can add a comment like this:
Function that is used to generate a digest on the Bloom Object.