redis-benchmark: Error/Warning handling updates. (#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.

1 files changed, 40 insertions, 42 deletions

diff --git a/src/redis-benchmark.c b/src/redis-benchmark.c
index 9aff2966a..a85c6610c 100644
--- a/src/redis-benchmark.c
+++ b/src/redis-benchmark.c
@@ -99,7 +99,6 @@ static struct config {
     int randomkeys_keyspacelen;
     int keepalive;
     int pipeline;
-    int showerrors;
     long long start;
     long long totlatency;
     const char *title;
@@ -300,7 +299,9 @@ static redisContext *getRedisContext(const char *ip, int port,
                 fprintf(stderr, "Node %s:%d replied with error:\n%s\n", ip, port, reply->str);
             else
                 fprintf(stderr, "Node %s replied with error:\n%s\n", hostsocket, reply->str);
-            goto cleanup;
+            freeReplyObject(reply);
+            redisFree(ctx);
+            exit(1);
         }
         freeReplyObject(reply);
         return ctx;
@@ -359,9 +360,15 @@ fail:
     fprintf(stderr, "ERROR: failed to fetch CONFIG from ");
     if (hostsocket == NULL) fprintf(stderr, "%s:%d\n", ip, port);
     else fprintf(stderr, "%s\n", hostsocket);
+    int abort_test = 0;
+    if (!strncmp(reply->str,"NOAUTH",5) ||
+        !strncmp(reply->str,"WRONGPASS",9) ||
+        !strncmp(reply->str,"NOPERM",5))
+        abort_test = 1;
     freeReplyObject(reply);
     redisFree(c);
     freeRedisConfig(cfg);
+    if (abort_test) exit(1);
     return NULL;
 }
 static void freeRedisConfig(redisConfig *cfg) {
@@ -506,44 +513,39 @@ static void readHandler(aeEventLoop *el, int fd, void *privdata, int mask) {
                     exit(1);
                 }
                 redisReply *r = reply;
-                int is_err = (r->type == REDIS_REPLY_ERROR);
-
-                if (is_err && config.showerrors) {
-                    /* TODO: static lasterr_time not thread-safe */
-                    static time_t lasterr_time = 0;
-                    time_t now = time(NULL);
-                    if (lasterr_time != now) {
-                        lasterr_time = now;
-                        if (c->cluster_node) {
-                            printf("Error from server %s:%d: %s\n",
+                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 the error is not topology-update related then we
+                    * immediately exit to avoid false results. */
+                    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);
+                    } else {
+                        if (c->cluster_node) {
+                            printf("Error from server %s:%d: %s\n",
+                                c->cluster_node->ip,
+                                c->cluster_node->port,
+                                r->str);
                         } else printf("Error from server: %s\n", r->str);
-                    }
-                }
-
-                /* 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 (is_err && 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);
+                    }
                 }
 
                 freeReplyObject(reply);
@@ -1286,8 +1288,7 @@ static int fetchClusterSlotsConfiguration(client c) {
     atomicGetIncr(config.is_fetching_slots, is_fetching_slots, 1);
     if (is_fetching_slots) return -1; //TODO: use other codes || errno ?
     atomicSet(config.is_fetching_slots, 1);
-    if (config.showerrors)
-        printf("Cluster slots configuration changed, fetching new one...\n");
+    printf("WARNING: Cluster slots configuration changed, fetching new one...\n");
     const char *errmsg = "Failed to update cluster slots configuration";
     static dictType dtype = {
         dictSdsHash,               /* hash function */
@@ -1463,7 +1464,8 @@ int parseOptions(int argc, const char **argv) {
         } else if (!strcmp(argv[i],"-I")) {
             config.idlemode = 1;
         } else if (!strcmp(argv[i],"-e")) {
-            config.showerrors = 1;
+            printf("WARNING: -e option has been deprecated. "
+                   "We now immediatly exit on error to avoid false results.\n");
         } else if (!strcmp(argv[i],"-t")) {
             if (lastarg) goto invalid;
             /* We get the list of tests to run as a string in the form
@@ -1566,8 +1568,6 @@ usage:
 "  is executed. Default tests use this to hit random keys in the\n"
 "  specified range.\n"
 " -P <numreq>        Pipeline <numreq> requests. Default 1 (no pipeline).\n"
-" -e                 If server replies with errors, show them on stdout.\n"
-"                    (no more than 1 error per second is displayed)\n"
 " -q                 Quiet. Just show query/sec values\n"
 " --precision        Number of decimal places to display in latency output (default 0)\n"
 " --csv              Output in CSV format\n"
@@ -1692,7 +1692,6 @@ int main(int argc, const char **argv) {
     config.keepalive = 1;
     config.datasize = 3;
     config.pipeline = 1;
-    config.showerrors = 0;
     config.randomkeys = 0;
     config.randomkeys_keyspacelen = 0;
     config.quiet = 0;
@@ -1777,7 +1776,6 @@ int main(int argc, const char **argv) {
             getRedisConfig(config.hostip, config.hostport, config.hostsocket);
         if (config.redis_config == NULL) {
             fprintf(stderr, "WARN: could not fetch server CONFIG\n");
-            exit(1);
         }
     }
     if (config.num_threads > 0) {