Skip to content

Dual-channel-replication announces itself at replica-announce-ip if configured #2846

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: unstable
Choose a base branch
from
+24 −0
Open

Dual-channel-replication announces itself at replica-announce-ip if configured #2846

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions src/replication.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2973,6 +2973,14 @@ static int dualChannelReplHandleHandshake(connection *conn, sds *err) {
return C_ERR;
}

if (server.replica_announce_ip) {
Copy link
Member

@ranshid ranshid Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can ALWAYS include the ip-address in the first replconf and thus reduce the need to explicitly handle the second replconf error handing?

Copy link
Author

@jdheyburn jdheyburn Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to be consistent with the current pattern for how replication handles replica_announce_ip. I am unsure how the REPLCONF command would handle a null ip-address.

  • valkey/src/replication.c

    Lines 3725 to 3731 in e19ceb7

    /* Set the replica ip, so that primary's INFO command can list the
    * replica IP address port correctly in case of port forwarding or NAT.
    * Skip REPLCONF ip-address if there is no replica-announce-ip option set. */
    if (server.replica_announce_ip) {
    err = sendCommand(conn, "REPLCONF", "ip-address", server.replica_announce_ip, NULL);
    if (err) goto err;
    }

Copy link
Member

@ranshid ranshid Nov 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I meant that we could pass the replica IP address even when the parameter is not configured. but it is not that critical TBH

*err = sendCommand(conn, "REPLCONF", "ip-address", server.replica_announce_ip, NULL);
if (*err) {
dualChannelServerLog(LL_WARNING, "Sending command to primary in dual channel replication handshake: %s", *err);
return C_ERR;
}
}

if (connSetReadHandler(conn, dualChannelFullSyncWithPrimary) == C_ERR) {
char conninfo[CONN_INFO_LEN];
dualChannelServerLog(LL_WARNING, "Can't create readable event for SYNC: %s (%s)", strerror(errno),
Expand Down Expand Up @@ -3008,6 +3016,22 @@ static int dualChannelReplHandleReplconfReply(connection *conn, sds *err) {
*err);
return C_ERR;
}

/* If replica-announce-ip is configured, we sent an additional REPLCONF ip-address command
* and need to read its response as well. */
if (server.replica_announce_ip) {
sdsfree(*err);
*err = receiveSynchronousResponse(conn);
if (*err == NULL) {
dualChannelServerLog(LL_WARNING, "Primary did not respond to REPLCONF ip-address command during SYNC handshake");
return C_ERR;
}
if ((*err)[0] == '-') {
dualChannelServerLog(LL_WARNING, "Primary rejected REPLCONF ip-address: %s", *err);
return C_ERR;
}
}

if (connSyncWrite(conn, "SYNC\r\n", 6, server.repl_syncio_timeout * 1000) == -1) {
dualChannelServerLog(LL_WARNING, "I/O error writing to Primary: %s", connGetLastError(conn));
return C_ERR;
Expand Down