redis-benchmark: Error/Warning handling updates. by filipecosta90 · Pull Request #8869 · redis/redis

filipecosta90 · 2021-04-26T15:11:26Z

This PR fixes #5854 and #2337 and addresses some minor issue introduced on #8855.
It:

Immediately exit on errors that are not related to topology updates. ( fixes Need to add auth check in benchmark? #5854 ).
Deprecates the -e option ( retro compatible ) and warns that we now exit immediately on errors that are not related to topology updates.
Fixed wrongfully failing on config fetch error (warning only). This only affects RE. ( reverts When the password is wrong, redis-benchmark should exit #8855 )

Bottom line:

MOVED and ASK errors will not show any warning (unlike the throttled error with -e before).
CLUSTERDOWN still prints an error unconditionally and sleeps for 1 second.
other errors are fatal.

…opology updates. Fixed wrongfully failing on config fetch error (warning only).

src/redis-benchmark.c

oranagra · 2021-04-27T11:45:06Z

src/redis-benchmark.c

+                if (r->type == REDIS_REPLY_ERROR) {
+                    /* Try to update slots configuration if reply error is
+                    * MOVED/ASK/CLUSTERDOWN and the key(s) used by the command
+                    * contain(s) the slot hash tag. */
+                    if (c->cluster_node && c->staglen) {
+                        int fetch_slots = 0, do_wait = 0;
+                        if (!strncmp(r->str,"MOVED",5) || !strncmp(r->str,"ASK",3))
+                            fetch_slots = 1;
+                        else if (!strncmp(r->str,"CLUSTERDOWN",11)) {
+                            /* Usually the cluster is able to recover itself after
+                            * a CLUSTERDOWN error, so try to sleep one second
+                            * before requesting the new configuration. */
+                            fetch_slots = 1;
+                            do_wait = 1;
+                            printf("Error from server %s:%d: %s\n",
+                                c->cluster_node->ip,
+                                c->cluster_node->port,
+                                r->str);
+                        }
+                        if (do_wait) sleep(1);
+                        if (fetch_slots && !fetchClusterSlotsConfiguration(c))
+                            exit(1);


it's a little bit hard to review this diff. IIUC, it didn't really change (other than the comment below.
what changed is that it went inside an additional separate if (on r->type == REDIS_REPLY_ERROR instead of is_err). and the first part at the stop about showerrorsmoved to the bottom (as anelsewithout anif`).

so logically, the only changes in that huge diff block is that:

we don't have the possibility to print the error twice.

normally MOVED, and ASK will not print any error

we exit on any unexpected error (in addition to the failure to fetch cluster slots configuration)

let me know if there's anything else i'm missing that changed in that function.

p.s. maybe there was a possibility to arrange the code in a way that it'll create a smaller diff, but never mind now.

@oranagra thank you for the fast review. with regards to:

we don't have the possibility to print the error twice.
correct.

normally MOVED, and ASK will not print any error
Missed it. Fixed it on the latest commit. We now print the MOVED and ASK same a before

we exit on any unexpected error (in addition to the failure to fetch cluster slots configuration)
correct.

let me know if there's anything else i'm missing that changed in that function.
All covered above.
p.s. maybe there was a possibility to arrange the code in a way that it'll create a smaller diff, but never mind now.
I also wanted to reduce the changes to the minimal but the identation change makes the line number to be large.

do we wanna print an error on MOVED and ASK?
i suppose it'll be printed only once (per thread?) since we re-fetch the cluster configuration, right?

note another difference is that the double errors in the past were throttled to be printed once per second, now we don't have double error print cases, but also no throttling.

the exit works as a way of "throttling" the unexpected errors.

MOVED and ASK are maybe solved by fetchClusterSlotsConfiguration (so they won't flood the console)
@filipecosta90 can you test to confirm that?

and CLUSTERDOWN is in addition to that throttled by the sleep

@oranagra you're totally right on flooding the console.
So my question is:

should we print the MOVED/ASK? If yes then we need the double errors in the past were throttled to be printed once per second. If not then we're ok with the previous print position. WDYT?

$ ../../src/redis-benchmark -r 100000 -n 1000000000 -t set -p 30002 --cluster Cluster has 3 master nodes: Master 0: ff62a57dec6aae7bf257f32fc11f709c6a6529d5 127.0.0.1:30001 Master 1: b4e30ae77b444361619b8ec3d182c1e5056c2939 127.0.0.1:30002 Master 2: 7c2512ef5932a70149343780a555ff95bdde9a6c 127.0.0.1:30003 Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. 0.194)3)) Updating slots configuration. WARNING: Cluster slots configuration changed, fetching new one... Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration. Error from server 127.0.0.1:30002: MOVED 5461 127.0.0.1:30001. Updating slots configuration.

i think that MOVED and ASK are part of the protocol, and are handled correctly by the benchmark.
they can be completely silenced IMHO.
in fact, before this PR, they were also completely silence by default, right? (when the showerrors was false)

@oranagra fixed per PR review: printing only CLUSTERDOWN error message.

…changed lines to minimal.

- Immediately exit on errors that are not related to topology updates. - Deprecates the `-e` option ( retro compatible ) and warns that we now exit immediately on errors that are not related to topology updates. - Fixed wrongfully failing on config fetch error (warning only). This only affects RE. Bottom line: - MOVED and ASK errors will not show any warning (unlike the throttled error with `-e` before). - CLUSTERDOWN still prints an error unconditionally and sleeps for 1 second. - other errors are fatal. (cherry picked from commit ef6f902)

Redis Enterprise supports the CONFIG GET command, but it replies with am empty array since the save and appendonly configs are not supported. before this fix redis-benchmark would segfault for trying to access the error string on an array type reply. see #8869

- Immediately exit on errors that are not related to topology updates. - Deprecates the `-e` option ( retro compatible ) and warns that we now exit immediately on errors that are not related to topology updates. - Fixed wrongfully failing on config fetch error (warning only). This only affects RE. Bottom line: - MOVED and ASK errors will not show any warning (unlike the throttled error with `-e` before). - CLUSTERDOWN still prints an error unconditionally and sleeps for 1 second. - other errors are fatal.

Redis Enterprise supports the CONFIG GET command, but it replies with am empty array since the save and appendonly configs are not supported. before this fix redis-benchmark would segfault for trying to access the error string on an array type reply. see redis#8869

Redis Enterprise supports the CONFIG GET command, but it replies with am empty array since the save and appendonly configs are not supported. before this fix redis-benchmark would segfault for trying to access the error string on an array type reply. see #8869 (cherry picked from commit 4d1094e)

- Immediately exit on errors that are not related to topology updates. - Deprecates the `-e` option ( retro compatible ) and warns that we now exit immediately on errors that are not related to topology updates. - Fixed wrongfully failing on config fetch error (warning only). This only affects RE. Bottom line: - MOVED and ASK errors will not show any warning (unlike the throttled error with `-e` before). - CLUSTERDOWN still prints an error unconditionally and sleeps for 1 second. - other errors are fatal.

Redis Enterprise supports the CONFIG GET command, but it replies with am empty array since the save and appendonly configs are not supported. before this fix redis-benchmark would segfault for trying to access the error string on an array type reply. see redis#8869

redis-benchmark: Immediately exit on errors that are not related to t…

ff8250b

…opology updates. Fixed wrongfully failing on config fetch error (warning only).

filipecosta90 requested review from oranagra and yangbodong22011 April 26, 2021 15:11

filipecosta90 mentioned this pull request Apr 26, 2021

Add authentication check for redis-benchmark #2339

Closed

oranagra reviewed Apr 27, 2021

View reviewed changes

filipecosta90 added 2 commits April 27, 2021 14:47

Fixes per PR review: addressed formatting issue. Tried to reduce the …

27bcf88

…changed lines to minimal.

Fixes per PR review: printing MOVED/ASK/CLUSTERDOWN error message.

f085a22

filipecosta90 requested a review from oranagra April 27, 2021 13:57

Fixes per PR review: printing only CLUSTERDOWN error message.

273d15d

oranagra approved these changes Apr 28, 2021

View reviewed changes

oranagra merged commit ef6f902 into redis:unstable Apr 28, 2021

filipecosta90 mentioned this pull request Apr 28, 2021

redis-benchmark does not report error when no auth is provided #2337

Closed

filipecosta90 mentioned this pull request May 3, 2021

redis-benchmark silently fails on unknown Redis commands #2900

Closed

oranagra mentioned this pull request May 6, 2021

fix redis-benchmark to ignore unsupported configs #8916

Merged

enjoy-binbin mentioned this pull request May 28, 2021

Benchmark should exit when redisConnect* fail. #9001

Closed

soloestoy mentioned this pull request Aug 9, 2022

benchmark getRedisConfig exit only when meet NOAUTH error #11096

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

redis-benchmark: Error/Warning handling updates.#8869

redis-benchmark: Error/Warning handling updates.#8869
oranagra merged 4 commits intoredis:unstablefrom
filipecosta90:redis.benchmark.errors

filipecosta90 commented Apr 26, 2021 •

edited by oranagra

Loading

Uh oh!

Uh oh!

oranagra Apr 27, 2021

Uh oh!

filipecosta90 Apr 27, 2021

Uh oh!

oranagra Apr 27, 2021

Uh oh!

filipecosta90 Apr 27, 2021

Uh oh!

oranagra Apr 27, 2021

Uh oh!

filipecosta90 Apr 27, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

filipecosta90 commented Apr 26, 2021 • edited by oranagra Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

oranagra Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

filipecosta90 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

oranagra Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

filipecosta90 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

oranagra Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

filipecosta90 Apr 27, 2021

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

filipecosta90 commented Apr 26, 2021 •

edited by oranagra

Loading