From 54cca7da76f14045720b262dcd06f840cdbf5032 Mon Sep 17 00:00:00 2001 From: filipecosta90 Date: Thu, 14 Jan 2021 17:41:56 +0000 Subject: [PATCH 1/5] [add] Enable mixing key placeholder with string arguments --- client.cpp | 11 ++++-- protocol.cpp | 5 --- tests/test_requirements.txt | 2 -- tests/tests_oss_simple_flow.py | 61 ++++++++++++++++++++++++++++++++++ 4 files changed, 70 insertions(+), 9 deletions(-) diff --git a/client.cpp b/client.cpp index 82045ed0..6b54afaf 100755 --- a/client.cpp +++ b/client.cpp @@ -50,6 +50,7 @@ #include "client.h" #include "cluster_client.h" +#include "config_types.h" bool client::setup_client(benchmark_config *config, abstract_protocol *protocol, object_generator *objgen) @@ -256,8 +257,14 @@ void client::create_arbitrary_request(const arbitrary_command* cmd, struct timev assert(key != NULL); assert(key_len > 0); - - cmd_size += m_connections[conn_id]->send_arbitrary_command(arg, key, key_len); + //when we have static data mixed with the key placeholder + if (arg->data.length() != strlen(KEY_PLACEHOLDER)) { + std::string str (arg->data); + str.replace(str.find(KEY_PLACEHOLDER),strlen(KEY_PLACEHOLDER),key); + cmd_size += m_connections[conn_id]->send_arbitrary_command(arg, str.c_str(), str.length() ); + } else{ + cmd_size += m_connections[conn_id]->send_arbitrary_command(arg, key, key_len); + } } else if (arg->type == data_type) { unsigned int value_len; const char *value = m_obj_gen->get_value(0, &value_len); diff --git a/protocol.cpp b/protocol.cpp index 51ac53a1..b922a302 100644 --- a/protocol.cpp +++ b/protocol.cpp @@ -631,11 +631,6 @@ bool redis_protocol::format_arbitrary_command(arbitrary_command &cmd) { // check arg type if (current_arg->data.find(KEY_PLACEHOLDER) != std::string::npos) { - if (current_arg->data.length() != strlen(KEY_PLACEHOLDER)) { - benchmark_error_log("error: key placeholder can't combined with other data\n"); - return false; - } - current_arg->type = key_type; } else if (current_arg->data.find(DATA_PLACEHOLDER) != std::string::npos) { if (current_arg->data.length() != strlen(DATA_PLACEHOLDER)) { diff --git a/tests/test_requirements.txt b/tests/test_requirements.txt index 8a689bff..41deaa97 100644 --- a/tests/test_requirements.txt +++ b/tests/test_requirements.txt @@ -1,4 +1,2 @@ -redis>=3.0.0 -git+https://github.com/Grokzen/redis-py-cluster.git@master git+https://github.com/RedisLabsModules/RLTest.git@master git+https://github.com/RedisLabs/mbdirector.git@master \ No newline at end of file diff --git a/tests/tests_oss_simple_flow.py b/tests/tests_oss_simple_flow.py index 0d29fb03..5447990e 100644 --- a/tests/tests_oss_simple_flow.py +++ b/tests/tests_oss_simple_flow.py @@ -144,3 +144,64 @@ def test_default_set_get_3_runs(env): overall_request_count = agg_info_commandstats(master_nodes_connections, merged_command_stats) assert_minimum_memtier_outcomes(config, env, memtier_ok, merged_command_stats, overall_expected_request_count, overall_request_count) + + +def test_key_placeholder(env): + env.skipOnCluster() + run_count = 1 + benchmark_specs = {"name": env.testName, "args": ['--command=HSET __key__ f __data__']} + addTLSArgs(benchmark_specs, env) + config = get_default_memtier_config() + master_nodes_list = env.getMasterNodesList() + overall_expected_request_count = get_expected_request_count(config) * run_count + + add_required_env_arguments(benchmark_specs, config, env, master_nodes_list) + + # Create a temporary directory + test_dir = tempfile.mkdtemp() + + config = RunConfig(test_dir, env.testName, config, {}) + ensure_clean_benchmark_folder(config.results_dir) + + benchmark = Benchmark.from_json(config, benchmark_specs) + + # benchmark.run() returns True if the return code of memtier_benchmark was 0 + memtier_ok = benchmark.run() + debugPrintMemtierOnError(config, env, memtier_ok) + + master_nodes_connections = env.getOSSMasterNodesConnectionList() + merged_command_stats = {'cmdstat_hset': {'calls': 0}} + overall_request_count = agg_info_commandstats(master_nodes_connections, merged_command_stats) + assert_minimum_memtier_outcomes(config, env, memtier_ok, merged_command_stats, overall_expected_request_count, + overall_request_count) + + +# key placeholder combined with other data +def test_key_placeholder_togetherwithdata(env): + env.skipOnCluster() + run_count = 1 + benchmark_specs = {"name": env.testName, "args": ['--command=HSET prefix:__key__:suffix f __data__']} + addTLSArgs(benchmark_specs, env) + config = get_default_memtier_config() + master_nodes_list = env.getMasterNodesList() + overall_expected_request_count = get_expected_request_count(config) * run_count + + add_required_env_arguments(benchmark_specs, config, env, master_nodes_list) + + # Create a temporary directory + test_dir = tempfile.mkdtemp() + + config = RunConfig(test_dir, env.testName, config, {}) + ensure_clean_benchmark_folder(config.results_dir) + + benchmark = Benchmark.from_json(config, benchmark_specs) + + # benchmark.run() returns True if the return code of memtier_benchmark was 0 + memtier_ok = benchmark.run() + debugPrintMemtierOnError(config, env, memtier_ok) + + master_nodes_connections = env.getOSSMasterNodesConnectionList() + merged_command_stats = {'cmdstat_hset': {'calls': 0}} + overall_request_count = agg_info_commandstats(master_nodes_connections, merged_command_stats) + assert_minimum_memtier_outcomes(config, env, memtier_ok, merged_command_stats, overall_expected_request_count, + overall_request_count) From 85759ab3bd77748d4a79f8e5ec5c9ac29bda3210 Mon Sep 17 00:00:00 2001 From: filipecosta90 Date: Mon, 3 May 2021 13:41:43 +0100 Subject: [PATCH 2/5] [fix] Fixes per PR review: avoid str::replace during runtime and using precomputed prefix and suffix --- client.cpp | 6 ++++-- config_types.h | 3 +++ protocol.cpp | 14 +++++++++++++- 3 files changed, 20 insertions(+), 3 deletions(-) diff --git a/client.cpp b/client.cpp index 6b54afaf..4744f3b7 100755 --- a/client.cpp +++ b/client.cpp @@ -46,6 +46,7 @@ #include #include +#include #include #include "client.h" @@ -260,8 +261,9 @@ void client::create_arbitrary_request(const arbitrary_command* cmd, struct timev //when we have static data mixed with the key placeholder if (arg->data.length() != strlen(KEY_PLACEHOLDER)) { std::string str (arg->data); - str.replace(str.find(KEY_PLACEHOLDER),strlen(KEY_PLACEHOLDER),key); - cmd_size += m_connections[conn_id]->send_arbitrary_command(arg, str.c_str(), str.length() ); + std::ostringstream key_stream; + key_stream << arg->data_prefix << key << arg->data_suffix; + cmd_size += m_connections[conn_id]->send_arbitrary_command(arg, key_stream.str().c_str(), key_stream.str().length()); } else{ cmd_size += m_connections[conn_id]->send_arbitrary_command(arg, key, key_len); } diff --git a/config_types.h b/config_types.h index b2a92239..73b541c4 100644 --- a/config_types.h +++ b/config_types.h @@ -116,6 +116,9 @@ struct command_arg { command_arg(const char* arg, unsigned int arg_len) : type(undefined_type), data(arg, arg_len) {;} command_arg_type type; std::string data; + // the prefix and suffix strings are used for mixed key placeholder storing of substrings + std::string data_prefix; + std::string data_suffix; }; struct arbitrary_command { diff --git a/protocol.cpp b/protocol.cpp index b922a302..fa4bbf23 100644 --- a/protocol.cpp +++ b/protocol.cpp @@ -630,8 +630,20 @@ bool redis_protocol::format_arbitrary_command(arbitrary_command &cmd) { current_arg->type = const_type; // check arg type - if (current_arg->data.find(KEY_PLACEHOLDER) != std::string::npos) { + const std::size_t key_placeholder_start = current_arg->data.find(KEY_PLACEHOLDER); + if (key_placeholder_start != std::string::npos) { current_arg->type = key_type; + current_arg->data_prefix = ""; + current_arg->data_suffix = ""; + // check for prefix + if (key_placeholder_start > 0) { + current_arg->data_prefix = current_arg->data.substr(0,key_placeholder_start); + } + // check for sufix + const std::size_t suffix_start = strlen(KEY_PLACEHOLDER)+key_placeholder_start; + if (current_arg->data.length() > suffix_start) { + current_arg->data_suffix = current_arg->data.substr(suffix_start,current_arg->data.length()); + } } else if (current_arg->data.find(DATA_PLACEHOLDER) != std::string::npos) { if (current_arg->data.length() != strlen(DATA_PLACEHOLDER)) { benchmark_error_log("error: data placeholder can't combined with other data\n"); From df3ee810b28b35741f0802727ea161d548848eac Mon Sep 17 00:00:00 2001 From: fcostaoliveira Date: Tue, 5 Aug 2025 17:30:57 +0100 Subject: [PATCH 3/5] properly handling misses/hits on arbitrary commands --- client.cpp | 4 +-- protocol.cpp | 69 ++++++++++++++++++++++------------------------------ protocol.h | 4 +-- 3 files changed, 33 insertions(+), 44 deletions(-) diff --git a/client.cpp b/client.cpp index e4b6e239..86804863 100755 --- a/client.cpp +++ b/client.cpp @@ -505,13 +505,13 @@ void client::handle_response(unsigned int conn_id, struct timeval timestamp, case rt_arbitrary: { arbitrary_request *ar = static_cast(request); - // Detect cache misses for arbitrary commands + // Detect cache misses for arbitrary commands based on protocol response const arbitrary_command& cmd = get_arbitrary_command(ar->index); unsigned int hits = 0; unsigned int misses = 0; if (cmd.keys_count > 0) { // Only check for commands that access keys - if (response->is_cache_miss_for_command(cmd.command_name)) { + if (response->is_cache_miss()) { misses = cmd.keys_count; } else { hits = cmd.keys_count; diff --git a/protocol.cpp b/protocol.cpp index 205cb66f..890eb767 100644 --- a/protocol.cpp +++ b/protocol.cpp @@ -123,48 +123,37 @@ unsigned int protocol_response::get_hits(void) return m_hits; } -bool protocol_response::is_cache_miss_for_command(const std::string& command_name) +bool protocol_response::is_cache_miss() { - // Commands that should track cache misses when they return empty/null results - - // Hash commands - if (command_name == "HGET" || command_name == "HGETALL" || command_name == "HMGET" || - command_name == "HKEYS" || command_name == "HVALS" || command_name == "HEXISTS") { - - // For bulk string responses ($-1 = null, $0 = empty but exists) - // For array responses (*-1 = null, *0 = empty array = miss for HGETALL/HKEYS/HVALS) - // For integer responses (:0 = miss for HEXISTS) - if (m_status && m_status[0] == '$' && m_status[1] == '-') return true; // $-1 (null bulk) - if (m_status && m_status[0] == '*' && m_status[1] == '0') return true; // *0 (empty array) - if (m_status && m_status[0] == ':' && m_status[1] == '0') return true; // :0 (zero integer) + // Protocol-based miss detection - works for ALL Redis commands + // Based on Redis protocol response patterns, not specific commands + + if (!m_status) return false; // No status means no response yet + + // Check Redis protocol response types that indicate "empty" or "not found" + switch (m_status[0]) { + case '$': // Bulk string response + // $-1 = null bulk string (key doesn't exist) + if (m_status[1] == '-') return true; + // $0 = empty string (key exists but empty) - this is a HIT, not a miss + return false; + + case '*': // Array response + // *-1 = null array (shouldn't happen in practice) + // *0 = empty array (no elements found) - typically a miss + if (m_status[1] == '-' || m_status[1] == '0') return true; + return false; + + case ':': // Integer response + // :0 = zero (count/length is zero) - typically a miss + if (m_status[1] == '0') return true; + return false; + + case '+': // Simple string (OK, etc.) - not a miss + case '-': // Error response - not a miss + default: + return false; } - - // List commands - else if (command_name == "LLEN" || command_name == "LINDEX" || command_name == "LRANGE") { - if (m_status && m_status[0] == ':' && m_status[1] == '0') return true; // :0 for LLEN - if (m_status && m_status[0] == '$' && m_status[1] == '-') return true; // $-1 for LINDEX - if (m_status && m_status[0] == '*' && m_status[1] == '0') return true; // *0 for LRANGE - } - - // Set commands - else if (command_name == "SCARD" || command_name == "SMEMBERS" || command_name == "SISMEMBER") { - if (m_status && m_status[0] == ':' && m_status[1] == '0') return true; // :0 for SCARD/SISMEMBER - if (m_status && m_status[0] == '*' && m_status[1] == '0') return true; // *0 for SMEMBERS - } - - // Sorted set commands - else if (command_name == "ZCARD" || command_name == "ZRANGE" || command_name == "ZSCORE") { - if (m_status && m_status[0] == ':' && m_status[1] == '0') return true; // :0 for ZCARD - if (m_status && m_status[0] == '*' && m_status[1] == '0') return true; // *0 for ZRANGE - if (m_status && m_status[0] == '$' && m_status[1] == '-') return true; // $-1 for ZSCORE - } - - // String commands (MGET handled separately due to multiple keys) - else if (command_name == "GET") { - if (m_status && m_status[0] == '$' && m_status[1] == '-') return true; // $-1 (null bulk) - } - - return false; // Not a miss } void protocol_response::clear(void) diff --git a/protocol.h b/protocol.h index a0953d69..7ffae328 100644 --- a/protocol.h +++ b/protocol.h @@ -136,8 +136,8 @@ struct protocol_response { void incr_hits(void); unsigned int get_hits(void); - // Check if response indicates a cache miss for arbitrary commands - bool is_cache_miss_for_command(const std::string& command_name); + // Check if response indicates a cache miss based on Redis protocol patterns + bool is_cache_miss(); void clear(); From 0bede38fbaafd09348865f498f0d29d70d927e6c Mon Sep 17 00:00:00 2001 From: fcostaoliveira Date: Thu, 7 Aug 2025 14:32:59 +0100 Subject: [PATCH 4/5] fixing debugPrint on error of tests --- tests/tests_oss_simple_flow.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/tests_oss_simple_flow.py b/tests/tests_oss_simple_flow.py index b10cdb53..7615cb0e 100644 --- a/tests/tests_oss_simple_flow.py +++ b/tests/tests_oss_simple_flow.py @@ -634,12 +634,12 @@ def test_key_placeholder(env): # benchmark.run() returns True if the return code of memtier_benchmark was 0 memtier_ok = benchmark.run() - debugPrintMemtierOnError(config, env, memtier_ok) + debugPrintMemtierOnError(config, env) master_nodes_connections = env.getOSSMasterNodesConnectionList() merged_command_stats = {'cmdstat_hset': {'calls': 0}} overall_request_count = agg_info_commandstats(master_nodes_connections, merged_command_stats) - assert_minimum_memtier_outcomes(config, env, memtier_ok, merged_command_stats, overall_expected_request_count, + assert_minimum_memtier_outcomes(config, env, memtier_ok, overall_expected_request_count, overall_request_count) @@ -665,12 +665,12 @@ def test_key_placeholder_togetherwithdata(env): # benchmark.run() returns True if the return code of memtier_benchmark was 0 memtier_ok = benchmark.run() - debugPrintMemtierOnError(config, env, memtier_ok) + debugPrintMemtierOnError(config, env) master_nodes_connections = env.getOSSMasterNodesConnectionList() merged_command_stats = {'cmdstat_hset': {'calls': 0}} overall_request_count = agg_info_commandstats(master_nodes_connections, merged_command_stats) - assert_minimum_memtier_outcomes(config, env, memtier_ok, merged_command_stats, overall_expected_request_count, + assert_minimum_memtier_outcomes(config, env, memtier_ok, overall_expected_request_count, overall_request_count) @@ -853,7 +853,7 @@ def test_arbitrary_command_cache_miss_tracking(env): # benchmark.run() returns True if the return code of memtier_benchmark was 0 memtier_ok = benchmark.run() - debugPrintMemtierOnError(config, env, memtier_ok) + debugPrintMemtierOnError(config, env) # Check that the JSON output includes hit/miss statistics for arbitrary commands json_filename = '{0}/mb.json'.format(config.results_dir) From 7473712f0e189ece469ab45e2fc29360232a1bb1 Mon Sep 17 00:00:00 2001 From: fcostaoliveira Date: Thu, 7 Aug 2025 16:19:04 +0100 Subject: [PATCH 5/5] Fixed Zipfian support for arbitrary commands. --- config_types.cpp | 1 + memtier_benchmark.cpp | 34 +++++++++++++++++++++++++++++++--- 2 files changed, 32 insertions(+), 3 deletions(-) diff --git a/config_types.cpp b/config_types.cpp index 60ce4c90..7525253b 100644 --- a/config_types.cpp +++ b/config_types.cpp @@ -326,6 +326,7 @@ bool arbitrary_command::set_key_pattern(const char* pattern_str) { if (pattern_str[0] != 'R' && pattern_str[0] != 'G' && + pattern_str[0] != 'Z' && pattern_str[0] != 'S' && pattern_str[0] != 'P') { diff --git a/memtier_benchmark.cpp b/memtier_benchmark.cpp index 3c948923..09991220 100755 --- a/memtier_benchmark.cpp +++ b/memtier_benchmark.cpp @@ -997,7 +997,7 @@ static int config_parse_args(int argc, char *argv[], struct benchmark_config *cf // command configuration always applied on last configured command arbitrary_command& cmd = cfg->arbitrary_commands->get_last_command(); if (!cmd.set_key_pattern(optarg)) { - fprintf(stderr, "error: key-pattern for command %s must be in the format of [S/R/G/P].\n", cmd.command_name.c_str()); + fprintf(stderr, "error: key-pattern for command %s must be in the format of [S/R/Z/G/P].\n", cmd.command_name.c_str()); return -1; } break; @@ -1160,6 +1160,7 @@ void usage() { " --command-key-pattern Key pattern for the command (default: R):\n" " G for Gaussian distribution.\n" " R for uniform Random.\n" + " Z for zipf distribution (will limit keys to positive).\n" " S for Sequential.\n" " P for Parallel (Sequential were each client has a subset of the key-range).\n" "\n" @@ -1779,7 +1780,20 @@ int main(int argc, char *argv[]) obj_gen->set_key_range(cfg.key_minimum, cfg.key_maximum); } if (cfg.key_stddev>0 || cfg.key_median>0) { - if (cfg.key_pattern[key_pattern_set]!='G' && cfg.key_pattern[key_pattern_get]!='G') { + // Check if Gaussian distribution is used in global key patterns or arbitrary commands + bool has_gaussian = (cfg.key_pattern[key_pattern_set]=='G' || cfg.key_pattern[key_pattern_get]=='G'); + + // Also check if any arbitrary command uses Gaussian distribution + if (!has_gaussian && cfg.arbitrary_commands->is_defined()) { + for (size_t i = 0; i < cfg.arbitrary_commands->size(); i++) { + if (cfg.arbitrary_commands->at(i).key_pattern == 'G') { + has_gaussian = true; + break; + } + } + } + + if (!has_gaussian) { fprintf(stderr, "error: key-stddev and key-median are only allowed together with key-pattern set to G.\n"); usage(); } @@ -1790,7 +1804,21 @@ int main(int argc, char *argv[]) obj_gen->set_key_distribution(cfg.key_stddev, cfg.key_median); } obj_gen->set_expiry_range(cfg.expiry_range.min, cfg.expiry_range.max); - if (cfg.key_pattern[key_pattern_set] == 'Z' || cfg.key_pattern[key_pattern_get] == 'Z') { + + // Check if Zipfian distribution is needed for global key patterns or arbitrary commands + bool needs_zipfian = (cfg.key_pattern[key_pattern_set] == 'Z' || cfg.key_pattern[key_pattern_get] == 'Z'); + + // Also check if any arbitrary command uses Zipfian distribution + if (!needs_zipfian && cfg.arbitrary_commands->is_defined()) { + for (size_t i = 0; i < cfg.arbitrary_commands->size(); i++) { + if (cfg.arbitrary_commands->at(i).key_pattern == 'Z') { + needs_zipfian = true; + break; + } + } + } + + if (needs_zipfian) { if (cfg.key_zipf_exp == 0.0) { // user can't specify 0.0, so 0.0 means unset cfg.key_zipf_exp = 1.0;