Repository: valkey-io/valkey
File: src/cluster_legacy.h (C/C++)

Code before:
#ifndef CLUSTER_LEGACY_H
#define CLUSTER_LEGACY_H

#define CLUSTER_PORT_INCR 10000 /* Cluster port = baseport + PORT_INCR */

/* The following defines are amount of time, sometimes expressed as
 * multiplicators of the node timeout value (when ending with MULT). */
#define CLUSTER_FAIL_REPORT_VALIDITY_MULT 2  /* Fail report validity. */
#define CLUSTER_FAIL_UNDO_TIME_MULT 2        /* Undo fail if primary is back. */
#define CLUSTER_MF_TIMEOUT 5000              /* Milliseconds to do a manual failover. */
#define CLUSTER_MF_PAUSE_MULT 2              /* Primary pause manual failover mult. */
#define CLUSTER_REPLICA_MIGRATION_DELAY 5000 /* Delay for replica migration. */

/* Reasons why a replica is not able to failover. */
#define CLUSTER_CANT_FAILOVER_NONE 0
#define CLUSTER_CANT_FAILOVER_DATA_AGE 1
#define CLUSTER_CANT_FAILOVER_WAITING_DELAY 2
#define CLUSTER_CANT_FAILOVER_EXPIRED 3
#define CLUSTER_CANT_FAILOVER_WAITING_VOTES 4
#define CLUSTER_CANT_FAILOVER_RELOG_PERIOD 5 /* seconds. */

/* clusterState todo_before_sleep flags. */
#define CLUSTER_TODO_HANDLE_FAILOVER (1 << 0)
#define CLUSTER_TODO_UPDATE_STATE (1 << 1)
#define CLUSTER_TODO_SAVE_CONFIG (1 << 2)
#define CLUSTER_TODO_FSYNC_CONFIG (1 << 3)
#define CLUSTER_TODO_HANDLE_MANUALFAILOVER (1 << 4)

/* clusterLink encapsulates everything needed to talk with a remote node. */
typedef struct clusterLink {
    mstime_t ctime;                        /* Link creation time */
    connection *conn;                      /* Connection to remote node */
    list *send_msg_queue;                  /* List of messages to be sent */
    size_t head_msg_send_offset;           /* Number of bytes already sent of message at head of queue */
    unsigned long long send_msg_queue_mem; /* Memory in bytes used by message queue */
    char *rcvbuf;                          /* Packet reception buffer */
    size_t rcvbuf_len;                     /* Used size of rcvbuf */
    size_t rcvbuf_alloc;                   /* Allocated size of rcvbuf */
    clusterNode *node;                     /* Node related to this link. Initialized to NULL when unknown */
    int inbound;                           /* 1 if this link is an inbound link accepted from the related node */
} clusterLink;

/* Cluster node flags and macros. */
#define CLUSTER_NODE_PRIMARY (1 << 0)               /* The node is a primary */
#define CLUSTER_NODE_REPLICA (1 << 1)               /* The node is a replica */

Review comment (refactor):
This change makes sense to me but now I wonder if we should shrink this further to 1 second, instead? I don't think having a few more duplicated messages at 1HZ is going to be an issue for anyone but there are two good things IMO

1. we don't have to reason about the magic number of 2s and 4s
2. we get to see the system making progress by watching the message.

Using 1s also covers pretty much all cases as I don't see a reason for using a <1s node timeout, test or prod.

Code after:
#ifndef CLUSTER_LEGACY_H
#define CLUSTER_LEGACY_H

#define CLUSTER_PORT_INCR 10000 /* Cluster port = baseport + PORT_INCR */

/* The following defines are amount of time, sometimes expressed as
 * multiplicators of the node timeout value (when ending with MULT). */
#define CLUSTER_FAIL_REPORT_VALIDITY_MULT 2  /* Fail report validity. */
#define CLUSTER_FAIL_UNDO_TIME_MULT 2        /* Undo fail if primary is back. */
#define CLUSTER_MF_TIMEOUT 5000              /* Milliseconds to do a manual failover. */
#define CLUSTER_MF_PAUSE_MULT 2              /* Primary pause manual failover mult. */
#define CLUSTER_REPLICA_MIGRATION_DELAY 5000 /* Delay for replica migration. */

/* Reasons why a replica is not able to failover. */
#define CLUSTER_CANT_FAILOVER_NONE 0
#define CLUSTER_CANT_FAILOVER_DATA_AGE 1
#define CLUSTER_CANT_FAILOVER_WAITING_DELAY 2
#define CLUSTER_CANT_FAILOVER_EXPIRED 3
#define CLUSTER_CANT_FAILOVER_WAITING_VOTES 4
#define CLUSTER_CANT_FAILOVER_RELOG_PERIOD 1 /* seconds. */

/* clusterState todo_before_sleep flags. */
#define CLUSTER_TODO_HANDLE_FAILOVER (1 << 0)
#define CLUSTER_TODO_UPDATE_STATE (1 << 1)
#define CLUSTER_TODO_SAVE_CONFIG (1 << 2)
#define CLUSTER_TODO_FSYNC_CONFIG (1 << 3)
#define CLUSTER_TODO_HANDLE_MANUALFAILOVER (1 << 4)

/* clusterLink encapsulates everything needed to talk with a remote node. */
typedef struct clusterLink {
    mstime_t ctime;                        /* Link creation time */
    connection *conn;                      /* Connection to remote node */
    list *send_msg_queue;                  /* List of messages to be sent */
    size_t head_msg_send_offset;           /* Number of bytes already sent of message at head of queue */
    unsigned long long send_msg_queue_mem; /* Memory in bytes used by message queue */
    char *rcvbuf;                          /* Packet reception buffer */
    size_t rcvbuf_len;                     /* Used size of rcvbuf */
    size_t rcvbuf_alloc;                   /* Allocated size of rcvbuf */
    clusterNode *node;                     /* Node related to this link. Initialized to NULL when unknown */
    int inbound;                           /* 1 if this link is an inbound link accepted from the related node */
} clusterLink;

/* Cluster node flags and macros. */
#define CLUSTER_NODE_PRIMARY (1 << 0)               /* The node is a primary */
#define CLUSTER_NODE_REPLICA (1 << 1)               /* The node is a replica */