Enable arbitrary commands to be run on cluster mode #117

filipecosta90 · 2020-03-23T11:06:26Z

This PR enables running arbitrary commands on cluster mode and adds the required testing to it.
Example command:

memtier_benchmark -s 10.3.0.54 -p 12002 -c 50 -t 14 \
            --command="hset __key__ f1 v1 f2 v2 f3 v3 f4 v4 f5 v5 f6 v6 f7 v7" \
            --hide-histogram --test-time 180 --run-count 3 --cluster-mode

Given it detected the issue in #204 it fixes it as well.

yossigo

@filipecosta90 This looks good to me, with a few minor changes (and we'll need to rebase all the commits). In your example above you only specify a single __key__, I guess there should be no problem to use an arbitrary number of __key__ and __data__ arguments right?

client.cpp

cluster_client.cpp

filipecosta90 · 2020-09-15T14:16:21Z

@filipecosta90 This looks good to me, with a few minor changes (and we'll need to rebase all the commits). In your example above you only specify a single __key__, I guess there should be no problem to use an arbitrary number of __key__ and __data__ arguments right?

Yes, assuming that the keys are all in the same slot ( given that we request for a connection based on the key ). I'll make that change and mention that the first key position will always be the one used by memtier to internal map the command to the proper shard connection. WDYT?

filipecosta90 · 2022-04-23T18:53:44Z

@filipecosta90 This looks good to me, with a few minor changes (and we'll need to rebase all the commits). In your example above you only specify a single __key__, I guess there should be no problem to use an arbitrary number of __key__ and __data__ arguments right?

@yossigo I've added the multi-placeholder test on the last commit. I believe all issues have been solved and we're ready to review :)

yossigo

@filipecosta90 Had a quick look, seems OK to me.
@YaacovHazan Any other inputs?

YaacovHazan · 2022-05-04T11:28:02Z

@filipecosta90 Looks good. I see that in the example test you are still using one __key__. Do we support more than one __key__? if yes what user should expect when the __key__'s are mapped to different slots?

Did we test it? As far as I remember when we are running with -n, we are filling the queue per connection according to the requested number of requests.
So once you reach that limit, all the conns send the keys that they have in thier queue.
But now are you sure we are not hung? or sending more requests than needed?

filipecosta90 · 2022-05-04T11:39:10Z

@filipecosta90 Looks good. I see that in the example test you are still using one __key__. Do we support more than one __key__? if yes what user should expect when the __key__'s are mapped to different slots?

@YaacovHazan with the following example:

memtier_benchmark --command "MSET __key__ v __key__ v" -p 30001 --cluster-mode

the expected CROSSSLOT error is replied:

server 127.0.0.1:30001 handle error response: -CROSSSLOT Keys in request don't hash to the same slot

I believe this is the expected behavior correct?

filipecosta90 · 2022-05-04T12:45:03Z

@YaacovHazan I've added an extra test for multi-key placeholders in 6ef066a

on the tests we confirm that the number of requests matches the expected.

…ommand cluster benchmarks

filipecosta90 · 2022-06-15T14:29:49Z

@YaacovHazan / @yossigo followed the recommendations and key placeholder is only allowed to be used once per command.
One interesting thing that I saw on the cluster_client is that before 5150d09 and 2d86e8c it was issuing more than the N requested commands of the benchmark and that was because of the code that fills in the key_index_pool for slots that are not of that connection. This was why I needed to do the change in 2d86e8c .

YaacovHazan · 2022-06-16T12:00:06Z

@filipecosta90 why did you add "Don't fill key_index_pool for other connections on cluster client" commit, this is by design

filipecosta90 · 2022-06-18T16:56:32Z

@filipecosta90 why did you add "Don't fill key_index_pool for other connections on cluster client" commit, this is by design

@YaacovHazan if we kept the code as is it hanged indefinitely given we were generating random keys that did not belong to that slot and then stopped prior generating any other key for that connection. As soon as I removed the check for total generated keys that included the keys that were buffered we were then issuing more commands that were required. The solution, ( proper one IMHO ) is not to buffer any key that does not belong to that conn. In this manner, we've solved both issues.

YaacovHazan · 2022-06-21T07:38:09Z

@filipecosta90 assuming Cluster Mode with SET/GET (1 key command), the idea is that you as a client have some key generator based on the user requirements.

Once you generated a key you should deliver it, if currently, you are in a context of a connection that the key does not belong to, it doesn't mean you can't or don't want to send it. You should use it but with the right connection.

If we were not connection driven, you could think of it as you have some basic component that generates keys and sends them with the right connection based on the current Cluster topology (and not that one connection put keys for another connection).

Otherwise, you are impacting the randomization of the keys based on the connection (you will use more keys that route to a good connection than a bad one).
The implementation in Memtier-Benchmark is that you choose your key generator and follow that creates the requests.

Now for Cluster with Arbitrary-Command (Multiple keys), the current implementation will not work well and will cause the client to hang. As I said at the early stage of this PR, we should think and consider what is the right way to handle that.
But IMOH just dropping keys is not the right solution and if yes, maybe only for this case of arbitrary command in Cluster mode.

filipecosta90 · 2023-01-31T17:32:39Z

@filipecosta90 assuming Cluster Mode with SET/GET (1 key command), the idea is that you as a client have some key generator based on the user requirements.

Once you generated a key you should deliver it, if currently, you are in a context of a connection that the key does not belong to, it doesn't mean you can't or don't want to send it. You should use it but with the right connection.

If we were not connection driven, you could think of it as you have some basic component that generates keys and sends them with the right connection based on the current Cluster topology (and not that one connection put keys for another connection).

Otherwise, you are impacting the randomization of the keys based on the connection (you will use more keys that route to a good connection than a bad one). The implementation in Memtier-Benchmark is that you choose your key generator and follow that creates the requests.

Now for Cluster with Arbitrary-Command (Multiple keys), the current implementation will not work well and will cause the client to hang. As I said at the early stage of this PR, we should think and consider what is the right way to handle that. But IMOH just dropping keys is not the right solution and if yes, maybe only for this case of arbitrary command in Cluster mode.

@YaacovHazan given arbitrary commands won't allow multiple key placeholders as in the current implementation I guess the only missing concern that is still to be addressed is the part of dropping the keys from different slots when not in arbitrary command. If we fix it IMHO we can move forward with this PR without compromising any of the current usage of cluster mode. Agree? Trying to see if we can make this as simple as possible to merge and have this feature. We need this so much! =)

YaacovHazan · 2023-02-02T17:49:15Z

@filipecosta90 ,Not sure I got you about the last concern. I think that once we limit the arbitrary command to one key placeholder, the PR is ok.

Two more notes:

I do think that we need to come up with a complete solution and add the ability to have arbitrary commands with more than one key placeholder.
I know that @ushachar is working on a different approach to generating keys per connection, and it could be the basic step to achieve that.

…request count(when defined)

… bellow request count(when defined)" This reverts commit 19c079b.

…ommand

…y_command_hset and test_default_arbitrary_command_hset_multi_data_placeholders

…request count(when defined)

…ommand

…request count

…request

filipecosta90 · 2023-05-22T14:49:44Z

@YaacovHazan I've enabled all tests and updated based upon:

IMOH just dropping keys is not the right solution and if yes, maybe only for this case of arbitrary command in Cluster mode.

All is green now. Can you check it?

YaacovHazan · 2023-05-22T18:55:36Z

protocol.cpp

            if (current_arg->data.length() != strlen(KEY_PLACEHOLDER)) {
                benchmark_error_log("error: key placeholder can't combined with other data\n");
                return false;
            }
-
+            if (key_placeholder_count > 1) {


This should apply only to cluster mode. The non-cluster mode does support more than one key.

Also, "IMOH just dropping keys is not the right solution and if yes, maybe only for this case of arbitrary command in Cluster mode." was relevant for the initial PR where we allowed more than one key.
Since we agree to limited the arbitrary command in cluster mode for one key (for now) there is no need to "play" with the pool

This should apply only to cluster mode. The non-cluster mode does support more than one key.

@YaacovHazan both for standalone and cluster versions we don't allow for more than one key placeholder.

filipecosta90 · 2023-06-19T21:33:59Z

memtier_benchmark.cpp

@@ -1293,6 +1290,11 @@ int main(int argc, char *argv[])
            exit(1);
        }

+        // Cluster mode supports only a single key commands
+        if (cfg.cluster_mode && cfg.arbitrary_commands->at(i).keys_count != 1) {


@YaacovHazan we should change from !=1 to >1.
Example of a failing command that should work:

$ ./memtier_benchmark --cluster --command="SET key __data__" -p 30001 error: Cluster mode supports only a single key commands

and another keyless one

./memtier_benchmark --cluster --command="PUBLISH channel __data__" -p 30001 error: Cluster mode supports only a single key commands

I'm including that example on CI and addressing it.

@YaacovHazan I've added the test and confirmed behaviour is as expected with the code change.

codecov-commenter · 2023-06-19T22:23:09Z

Codecov Report

Merging #117 (b6c2158) into master (5c8be9c) will decrease coverage by 0.62%.
The diff coverage is 70.76%.

❗ Current head b6c2158 differs from pull request most recent head 9cd9c82. Consider uploading reports for the commit 9cd9c82 to get more accurate results

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@            Coverage Diff             @@
##           master     #117      +/-   ##
==========================================
- Coverage   56.87%   56.25%   -0.62%     
==========================================
  Files          21       21              
  Lines        4283     4330      +47     
==========================================
  Hits         2436     2436              
- Misses       1847     1894      +47

Impacted Files	Coverage Δ
config_types.h	`91.66% <ø> (-2.46%)`	⬇️
run_stats.h	`100.00% <ø> (ø)`
memtier_benchmark.cpp	`52.81% <50.00%> (+0.05%)`	⬆️
run_stats.cpp	`77.50% <50.00%> (-0.79%)`	⬇️
client.cpp	`63.92% <66.66%> (+0.74%)`	⬆️
cluster_client.cpp	`70.24% <77.77%> (+0.39%)`	⬆️
client.h	`75.00% <93.33%> (+12.50%)`	⬆️
config_types.cpp	`42.91% <100.00%> (+0.39%)`	⬆️
protocol.cpp	`36.65% <100.00%> (+0.10%)`	⬆️
shard_connection.cpp	`61.73% <100.00%> (+0.12%)`	⬆️

... and 1 file with indirect coverage changes

yossigo requested changes Sep 2, 2020

View reviewed changes

client.cpp Outdated Show resolved Hide resolved

cluster_client.cpp Outdated Show resolved Hide resolved

cluster_client.cpp Outdated Show resolved Hide resolved

yossigo mentioned this pull request Sep 2, 2020

i would like to know when memtier_benchmark would support arbitary command, even in cluster mode #127

Closed

filipecosta90 added the enhancement label Jan 14, 2021

filipecosta90 force-pushed the cluster.arbitrary.command branch from 20fd759 to 631b731 Compare May 3, 2021 14:14

filipecosta90 requested a review from yossigo May 3, 2021 14:15

filipecosta90 force-pushed the cluster.arbitrary.command branch from c627c75 to e3cac2f Compare March 14, 2022 10:03

filipecosta90 force-pushed the cluster.arbitrary.command branch from 3e9ef61 to 6cb6eef Compare April 21, 2022 09:24

Enable arbitrary commands to be run on cluster mode

3ab8c0f

filipecosta90 force-pushed the cluster.arbitrary.command branch from 6cb6eef to 3ab8c0f Compare April 21, 2022 09:55

filipecosta90 requested a review from YaacovHazan April 21, 2022 10:08

Added multi-placeholder test for arbitrary command

d74d161

yossigo previously approved these changes May 3, 2022

View reviewed changes

Added multi-key command test

6ef066a

filipecosta90 dismissed yossigo’s stale review via 6ef066a May 4, 2022 12:42

filipecosta90 added 2 commits June 15, 2022 15:08

Restrict key placeholder usage for 1 per command. Enabled arbitrary c…

5150d09

…ommand cluster benchmarks

Don't fill key_index_pool for other connections on cluster client

2d86e8c

filipecosta90 requested a review from yossigo June 15, 2022 14:26

filipecosta90 linked an issue Jun 18, 2022 that may be closed by this pull request

i would like to know when memtier_benchmark would support arbitary command, even in cluster mode #127

Closed

filipecosta90 added 2 commits February 2, 2023 14:09

Merge branch 'master' into cluster.arbitrary.command

26351b4

Removed unrequired changes on the PR

e10d66a

Making CI cluster tests more verbose

a443f2f

ensuring that when there are multiple shards the keys pool is bellow …

2b664dd

…request count(when defined)

filipecosta90 linked an issue Feb 6, 2023 that may be closed by this pull request

key pool can cause memtier to hang when there are multiple shards and request count is bellow pool size #204

Closed

filipecosta90 added 13 commits February 7, 2023 09:57

Revert "ensuring that when there are multiple shards the keys pool is…

f9f324b

… bellow request count(when defined)" This reverts commit 19c079b.

Merge remote-tracking branch 'origin/master' into cluster.arbitrary.c…

fa06f87

…ommand

Fixed assert_minimum_memtier_outcomes inputs in test_default_arbitrar…

9700d43

…y_command_hset and test_default_arbitrary_command_hset_multi_data_placeholders

Fixed debug print on new tests

ec241aa

Ensuring that when there are multiple shards the keys pool is bellow …

417ae5c

…request count(when defined)

Merge remote-tracking branch 'origin/master' into cluster.arbitrary.c…

a51f434

…ommand

Revert key pool changes

5d0b8f2

Increase request count on oss cluster benchmarks

ae39cb6

Increase request count on oss cluster benchmarks

b1c31fe

Cleaned unit tests

1433b7d

ensuring that when there are multiple shards the keys pool is bellow …

643a1b3

…request count

Don't store key from different slot in pool if cluster and arbitrary …

7e79da5

…request

Enabled all tests on cluster mode

288876a

YaacovHazan requested changes May 22, 2023

View reviewed changes

fix support for cluster mode and arbitrary commands

5247d3d

filipecosta90 requested a review from YaacovHazan June 19, 2023 21:22

filipecosta90 commented Jun 19, 2023

View reviewed changes

Included keyless command test. always consume generated keys

5e6175f

fix cluster mode with keyless command

9cd9c82

YaacovHazan approved these changes Jun 21, 2023

View reviewed changes

filipecosta90 merged commit 1c6735f into RedisLabs:master Jun 21, 2023
3 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enable arbitrary commands to be run on cluster mode #117

Enable arbitrary commands to be run on cluster mode #117

filipecosta90 commented Mar 23, 2020 •

edited

Loading

yossigo left a comment

filipecosta90 commented Sep 15, 2020

filipecosta90 commented Apr 23, 2022

yossigo left a comment

YaacovHazan commented May 4, 2022 •

edited

Loading

filipecosta90 commented May 4, 2022

filipecosta90 commented May 4, 2022

filipecosta90 commented Jun 15, 2022

YaacovHazan commented Jun 16, 2022

filipecosta90 commented Jun 18, 2022

YaacovHazan commented Jun 21, 2022

filipecosta90 commented Jan 31, 2023

YaacovHazan commented Feb 2, 2023

filipecosta90 commented May 22, 2023

YaacovHazan May 22, 2023

YaacovHazan May 22, 2023

filipecosta90 May 23, 2023

filipecosta90 Jun 19, 2023 •

edited

Loading

filipecosta90 Jun 19, 2023

codecov-commenter commented Jun 19, 2023 •

edited

Loading

Enable arbitrary commands to be run on cluster mode #117

Enable arbitrary commands to be run on cluster mode #117

Conversation

filipecosta90 commented Mar 23, 2020 • edited Loading

yossigo left a comment

Choose a reason for hiding this comment

filipecosta90 commented Sep 15, 2020

filipecosta90 commented Apr 23, 2022

yossigo left a comment

Choose a reason for hiding this comment

YaacovHazan commented May 4, 2022 • edited Loading

filipecosta90 commented May 4, 2022

filipecosta90 commented May 4, 2022

filipecosta90 commented Jun 15, 2022

YaacovHazan commented Jun 16, 2022

filipecosta90 commented Jun 18, 2022

YaacovHazan commented Jun 21, 2022

filipecosta90 commented Jan 31, 2023

YaacovHazan commented Feb 2, 2023

filipecosta90 commented May 22, 2023

YaacovHazan May 22, 2023

Choose a reason for hiding this comment

YaacovHazan May 22, 2023

Choose a reason for hiding this comment

filipecosta90 May 23, 2023

Choose a reason for hiding this comment

filipecosta90 Jun 19, 2023 • edited Loading

Choose a reason for hiding this comment

filipecosta90 Jun 19, 2023

Choose a reason for hiding this comment

codecov-commenter commented Jun 19, 2023 • edited Loading

Codecov Report

filipecosta90 commented Mar 23, 2020 •

edited

Loading

YaacovHazan commented May 4, 2022 •

edited

Loading

filipecosta90 Jun 19, 2023 •

edited

Loading

codecov-commenter commented Jun 19, 2023 •

edited

Loading