Fixes per PR review: refactored retry logic to eliminate code duplication.

Author: fcostaoliveira

.gitignore

@@ -6,6 +6,7 @@ config.h.in~
 config.h
 config.sub
 configure
+configure~
 compile
 depcomp
 install-sh

memtier_benchmark.cpp

@@ -76,6 +76,7 @@ static void sigint_handler(int signum)
     (void)signum;  // unused parameter
     g_interrupted = 1;
 }
+
 void benchmark_log_file_line(int level, const char *filename, unsigned int line, const char *fmt, ...)
 {
     if (level > log_level)
@@ -459,10 +460,6 @@ static void config_init_defaults(struct benchmark_config *cfg)
     if (!cfg->print_percentiles.is_defined())
         cfg->print_percentiles = config_quantiles("50,99,99.9");
 
-    // Set defaults for reconnection on error
-    // Note: These defaults are only applied when reconnect_on_error is enabled
-    // connection_timeout defaults to 0 (disabled) for backwards compatibility
-
 #ifdef USE_TLS
     if (!cfg->tls_protocols)
         cfg->tls_protocols = REDIS_TLS_PROTO_DEFAULT;

shard_connection.cpp

@@ -664,99 +664,62 @@ void shard_connection::handle_event(short events)
             benchmark_error_log("Connection error: %s\n", strerror(errno));
         }
 
-        // Update connection error statistics
-        struct timeval now;
-        gettimeofday(&now, NULL);
-        client* c = static_cast<client*>(m_conns_manager);
-        c->get_stats()->update_connection_error(&now);
-
-        // Attempt reconnection if enabled and not already reconnecting
-        if (m_config->reconnect_on_error && !m_reconnecting &&
-            (m_config->max_reconnect_attempts == 0 || m_reconnect_attempts < m_config->max_reconnect_attempts)) {
-
-            disconnect();
-            m_reconnect_attempts++;
-            if (m_config->reconnect_backoff_factor > 0.0) {
-                m_current_backoff_delay *= m_config->reconnect_backoff_factor;
-            }
-
-            if (m_config->max_reconnect_attempts == 0) {
-                benchmark_error_log("Connection error, attempting reconnection %u (unlimited) in %.2f seconds...\n",
-                                  m_reconnect_attempts, m_current_backoff_delay);
-            } else {
-                benchmark_error_log("Connection error, attempting reconnection %u/%u in %.2f seconds...\n",
-                                  m_reconnect_attempts, m_config->max_reconnect_attempts, m_current_backoff_delay);
-            }
-
-            // Schedule reconnection attempt
-            struct timeval delay;
-            delay.tv_sec = (long)m_current_backoff_delay;
-            delay.tv_usec = (long)((m_current_backoff_delay - delay.tv_sec) * 1000000);
-
-            m_reconnect_timer = event_new(m_event_base, -1, 0, cluster_client_reconnect_timer_handler, (void *)this);
-            event_add(m_reconnect_timer, &delay);
-            m_reconnecting = true;
-        } else {
-            benchmark_error_log("Maximum reconnection attempts (%u) exceeded for connection error, triggering thread restart.\n",
-                              m_config->max_reconnect_attempts);
-            disconnect();
-            // Break the event loop to trigger thread restart
-            event_base_loopbreak(m_event_base);
-        }
-
+        attempt_reconnect("Connection error");
         return;
     }
 
     if (events & BEV_EVENT_EOF) {
         benchmark_error_log("connection dropped.\n");
+        attempt_reconnect("Connection dropped");
+        return;
+    }
+}
 
-        // Update connection error statistics
-        struct timeval now;
-        gettimeofday(&now, NULL);
-        client* c = static_cast<client*>(m_conns_manager);
-        c->get_stats()->update_connection_error(&now);
-
-        // Attempt reconnection if enabled and not already reconnecting
-        if (m_config->reconnect_on_error && !m_reconnecting &&
-            (m_config->max_reconnect_attempts == 0 || m_reconnect_attempts < m_config->max_reconnect_attempts)) {
+void shard_connection::handle_timer_event() {
+    m_request_per_cur_interval = m_config->request_per_interval;
+    fill_pipeline();
+}
 
-            disconnect();
-            m_reconnect_attempts++;
-            if (m_config->reconnect_backoff_factor > 0.0) {
-                m_current_backoff_delay *= m_config->reconnect_backoff_factor;
-            }
+void shard_connection::attempt_reconnect(const char* error_context) {
+    // Update connection error statistics
+    struct timeval now;
+    gettimeofday(&now, NULL);
+    client* c = static_cast<client*>(m_conns_manager);
+    c->get_stats()->update_connection_error(&now);
 
-            if (m_config->max_reconnect_attempts == 0) {
-                benchmark_error_log("Connection dropped, attempting reconnection %u (unlimited) in %.2f seconds...\n",
-                                  m_reconnect_attempts, m_current_backoff_delay);
-            } else {
-                benchmark_error_log("Connection dropped, attempting reconnection %u/%u in %.2f seconds...\n",
-                                  m_reconnect_attempts, m_config->max_reconnect_attempts, m_current_backoff_delay);
-            }
+    // Attempt reconnection if enabled and not already reconnecting
+    if (m_config->reconnect_on_error && !m_reconnecting &&
+        (m_config->max_reconnect_attempts == 0 || m_reconnect_attempts < m_config->max_reconnect_attempts)) {
 
-            // Schedule reconnection attempt
-            struct timeval delay;
-            delay.tv_sec = (long)m_current_backoff_delay;
-            delay.tv_usec = (long)((m_current_backoff_delay - delay.tv_sec) * 1000000);
+        disconnect();
+        m_reconnect_attempts++;
+        if (m_config->reconnect_backoff_factor > 0.0) {
+            m_current_backoff_delay *= m_config->reconnect_backoff_factor;
+        }
 
-            m_reconnect_timer = event_new(m_event_base, -1, 0, cluster_client_reconnect_timer_handler, (void *)this);
-            event_add(m_reconnect_timer, &delay);
-            m_reconnecting = true;
+        if (m_config->max_reconnect_attempts == 0) {
+            benchmark_error_log("%s, attempting reconnection %u (unlimited) in %.2f seconds...\n",
+                              error_context, m_reconnect_attempts, m_current_backoff_delay);
         } else {
-            benchmark_error_log("Maximum reconnection attempts (%u) exceeded for connection drop, triggering thread restart.\n",
-                              m_config->max_reconnect_attempts);
-            disconnect();
-            // Break the event loop to trigger thread restart
-            event_base_loopbreak(m_event_base);
+            benchmark_error_log("%s, attempting reconnection %u/%u in %.2f seconds...\n",
+                              error_context, m_reconnect_attempts, m_config->max_reconnect_attempts, m_current_backoff_delay);
         }
 
-        return;
-    }
-}
+        // Schedule reconnection attempt
+        struct timeval delay;
+        delay.tv_sec = (long)m_current_backoff_delay;
+        delay.tv_usec = (long)((m_current_backoff_delay - delay.tv_sec) * 1000000);
 
-void shard_connection::handle_timer_event() {
-    m_request_per_cur_interval = m_config->request_per_interval;
-    fill_pipeline();
+        m_reconnect_timer = event_new(m_event_base, -1, 0, cluster_client_reconnect_timer_handler, (void *)this);
+        event_add(m_reconnect_timer, &delay);
+        m_reconnecting = true;
+    } else {
+        benchmark_error_log("Maximum reconnection attempts (%u) exceeded for %s, triggering thread restart.\n",
+                          m_config->max_reconnect_attempts, error_context);
+        disconnect();
+        // Break the event loop to trigger thread restart
+        event_base_loopbreak(m_event_base);
+    }
 }
 
 void shard_connection::handle_reconnect_timer_event() {
@@ -815,46 +778,7 @@ void shard_connection::handle_connection_timeout_event() {
     }
 
     benchmark_error_log("Connection timeout after %u seconds.\n", m_config->connection_timeout);
-
-    // Update connection error statistics
-    struct timeval now;
-    gettimeofday(&now, NULL);
-    client* c = static_cast<client*>(m_conns_manager);
-    c->get_stats()->update_connection_error(&now);
-
-    // Treat timeout as a connection error and attempt reconnection if enabled
-    if (m_config->reconnect_on_error && !m_reconnecting &&
-        (m_config->max_reconnect_attempts == 0 || m_reconnect_attempts < m_config->max_reconnect_attempts)) {
-
-        disconnect();
-        m_reconnect_attempts++;
-        if (m_config->reconnect_backoff_factor > 0.0) {
-            m_current_backoff_delay *= m_config->reconnect_backoff_factor;
-        }
-
-        if (m_config->max_reconnect_attempts == 0) {
-            benchmark_error_log("Connection timeout, attempting reconnection %u (unlimited) in %.2f seconds...\n",
-                              m_reconnect_attempts, m_current_backoff_delay);
-        } else {
-            benchmark_error_log("Connection timeout, attempting reconnection %u/%u in %.2f seconds...\n",
-                              m_reconnect_attempts, m_config->max_reconnect_attempts, m_current_backoff_delay);
-        }
-
-        // Schedule reconnection attempt
-        struct timeval delay;
-        delay.tv_sec = (long)m_current_backoff_delay;
-        delay.tv_usec = (long)((m_current_backoff_delay - delay.tv_sec) * 1000000);
-
-        m_reconnect_timer = event_new(m_event_base, -1, 0, cluster_client_reconnect_timer_handler, (void *)this);
-        event_add(m_reconnect_timer, &delay);
-        m_reconnecting = true;
-    } else {
-        benchmark_error_log("Maximum reconnection attempts (%u) exceeded for connection timeout, triggering thread restart.\n",
-                          m_config->max_reconnect_attempts);
-        disconnect();
-        // Break the event loop to trigger thread restart
-        event_base_loopbreak(m_event_base);
-    }
+    attempt_reconnect("Connection timeout");
 }
 
 void shard_connection::send_wait_command(struct timeval* sent_time,

shard_connection.h

@@ -153,6 +153,7 @@ class shard_connection {
 
     void handle_event(short evtype);
     void handle_timer_event();
+    void attempt_reconnect(const char* error_context);
 
     unsigned int m_id;
     connections_manager* m_conns_manager;