Implement failover and replicaof command by prateek-kumar-improving · Pull Request #282 · valkey-io/valkey-glide-php

prateek-kumar-improving · 2026-07-27T22:23:34Z

Summary

Implement REPLICAOF and FAILOVER commands for the standalone ValkeyGlide client.

Issue link

This Pull Request is linked to issue: #236

Features / Behaviour Changes

replicaof(?string $host = null, int $port = 6379) — Makes the server a replica of the specified primary. When called with null host, sends REPLICAOF NO ONE to promote the server to a primary.
failover(?array $to = null, bool $abort = false, int $timeout = 0) — Starts a coordinated failover from the connected primary to one of its replicas. Supports TO host port [FORCE], ABORT, and TIMEOUT options.
Both commands support OPT_REPLY_LITERAL (returns "OK" string instead of true).
Standalone-only commands — not exposed on ValkeyGlideCluster (matching PHPRedis behavior).

Implementation

Added execute_failover_command() and execute_replicaof_command() in valkey_glide_commands.c
Both use STANDARD_METHOD_IMPL macro pattern
Commands registered as FailOver (1127) and ReplicaOf (1151) request types in glide-core
Added to prepare_message_args switch case in valkey_glide_core_common.c (alphabetized)
Added independent Valkey server on port 6382 in CI for isolated testing

Limitations

FAILOVER success test performs best-effort topology restoration after failover completes. The glide client connection may become unstable after a role change.
Cluster mode does not support these commands (standalone-only).

Testing

All tests are skipped in cluster mode via overrides in ValkeyGlideClusterTest.

Checklist

Before submitting the PR make sure the following are checked:

This Pull Request is related to one issue.
Commit message has a detailed description of what changed and why.
Tests are added or updated.
CHANGELOG.md and documentation files are updated.
Destination branch is correct - main or release
Create merge commit if merging release branch into main, squash otherwise.

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Signed-off-by: prateek-kumar-improving <prateek.kumar@improving.com>

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Signed-off-by: prateek-kumar-improving <prateek.kumar@improving.com>

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Signed-off-by: prateek-kumar-improving <prateek.kumar@improving.com>

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

nderraugh · 2026-07-31T14:25:47Z

+        $this->assertGTE(1, (int) $info['connected_slaves']);
+
+        // FAILOVER returns true (command accepted, failover is async)
+        $result = $client->failover();


Does anything need to get reset with the servers after this test succeeds or fails after an attempted failover so that other following tests aren't affected? i.e. something to run in a tear down-like phase to reset the servers.

nderraugh · 2026-07-31T14:41:33Z

+
+    int arg_idx = 0;
+
+    if (abort) {


abort doesn't use the timeout parameter?

If these parameters (host, abort, timeout) have conflicting permutations, it might be worth addressing them the same way it's done in other places.

i.e. @throws ValkeyGlideException If conflicting parameters are specified or connection fails.

Alternatively succeeding and ignoring the conflict might be worth logging. Or perhaps even splitting functions along the abort path.

nderraugh · 2026-07-31T15:29:51Z

+            zval* z_host = zend_hash_str_find(Z_ARRVAL_P(to_arr), "host", 4);
+            zval* z_port = zend_hash_str_find(Z_ARRVAL_P(to_arr), "port", 4);
+
+            if (!z_host || Z_TYPE_P(z_host) != IS_STRING || !z_port) {


Similar issue around validation here.

Existing validation patterns look like this

Suggested change

if (!z_host || Z_TYPE_P(z_host) != IS_STRING || !z_port) {

if (!z_host || Z_TYPE_P(z_host) != IS_STRING || !z_port) {

VALKEY_LOG_ERROR("command_validation", "'to' array must contain string 'host' and 'port'");

zend_throw_exception(get_valkey_glide_exception_ce(),

"'to' array must contain string 'host' and 'port'", 0);

I think it makes sense to use that here. Otherwise a malformed $to fails silently with no hint about the problem.

Might be worth thinking about whether this can be done cohesively around lines 737-740.

nderraugh · 2026-07-31T15:35:36Z

+        core_args.args[1].data.string_arg.len   = 3;
+        core_args.arg_count                     = 2;
+    } else {
+        /* REPLICAOF host port */


Do we need to do any validation on host here? e.g. should '' be normalized or rejected (and logged iwth explanation) or something else?

currantw

Reviewed to 49822f9.

currantw · 2026-07-31T16:18:13Z

+    /**
+     * Starts a coordinated failover from the connected primary to one of its replicas.
+     *
+     * @param array|null $to    Target replica as ['host' => string, 'port' => int], or null for auto-selection.


Nit.

Suggested change

* @param array|null $to Target replica as ['host' => string, 'port' => int], or null for auto-selection.

* @param array|null $to Target replica as ['host' => string, 'port' => int], or null for auto-selection.

currantw · 2026-07-31T16:33:00Z

 /* }}} */

-/* {{{ proto string ValkeyGlide::bgSave([string mode]) */
+/* {{{ proto bool ValkeyGlide::bgSave([string mode]) */


Thanks for fixing 💪

currantw · 2026-07-31T16:35:09Z

+        // Verify it's currently a master
+        $info = $client->info('REPLICATION');
+        $this->assertEquals('master', $info['role']);


Can we use waitForRole here?

(Or assertRole comments suggested in other comment)

currantw · 2026-07-31T16:35:50Z

+        // Verify role changed back to master
+        $info = $client->info('REPLICATION');
+        $this->assertEquals('master', $info['role']);


Can we use waitForRole here?

(Or assertRole comments suggested in other comment)

currantw · 2026-07-31T16:36:30Z

+        $client->setOption(ValkeyGlide::OPT_REPLY_LITERAL, true);
+        try {
+            $result = $client->replicaof('127.0.0.1', 6379);
+            $this->assertEquals('OK', $result);
+
+            // Wait for replication to establish
+            $client->setOption(ValkeyGlide::OPT_REPLY_LITERAL, false);
+            $this->waitForRole($client, 'slave');
+            $client->setOption(ValkeyGlide::OPT_REPLY_LITERAL, true);
+
+            $result = $client->replicaof();
+            $this->assertEquals('OK', $result);
+        } finally {
+            $client->setOption(ValkeyGlide::OPT_REPLY_LITERAL, false);
+        }


Didn't we add a helper to simplify the OPT_REPLY_LITERAL cases?

currantw · 2026-07-31T16:36:41Z

+        // Verify role restored to master
+        $info = $client->info('REPLICATION');
+        $this->assertEquals('master', $info['role']);
+


Can we use waitForRole here?

(Or assertRole comments suggested in other comment)

currantw · 2026-07-31T16:37:14Z

+            $this->assertFalse($result);
+        } finally {
+            $client->setOption(ValkeyGlide::OPT_REPLY_LITERAL, false);
+        }


Similar comment about using OPT_REPLY_LITERAL helper.

currantw · 2026-07-31T16:37:49Z

+        $info = $client->info('REPLICATION');
+        $this->assertEquals('master', $info['role']);


Use waitForRole here?

(Or assertRole comments suggested in other comment)

currantw · 2026-07-31T16:39:22Z

+    /**
+     * Poll until a server reaches the expected role, or fail after timeout.
+     */
+    protected function waitForRole(ValkeyGlide $client, string $expectedRole, int $timeoutMs = 5000): void


Do we have a generic waitFor base method that this could use? We have something similar in the other clients. If not, I think it makes sense to add it?

We should also add an assertRole method. This could be used for cases where we don't need to wait and passed to waitFor for the case where we do?

Implement failover and replicaof command

e3c90dd

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving added the command label Jul 27, 2026

Fix tests

6d5df71

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving self-assigned this Jul 27, 2026

prateek-kumar-improving added 3 commits July 28, 2026 01:15

Fix tests

e467c98

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Update changelog

9a9cb5b

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Fix tests

cbe7206

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving mentioned this pull request Jul 28, 2026

Add commands for compatibility with other GLIDE clients #236

Open

20 tasks

Fix tests

f8648c8

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

prateek-kumar-improving marked this pull request as ready for review July 28, 2026 17:55

prateek-kumar-improving added 2 commits July 29, 2026 00:57

Merge branch 'main' into implement-replicaof-failover-commands

d8d22ff

Signed-off-by: prateek-kumar-improving <prateek.kumar@improving.com>

Fix lint error

528f35a

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

yipin-chen requested review from currantw and nderraugh July 30, 2026 17:06

prateek-kumar-improving added 8 commits July 30, 2026 10:44

Merge branch 'main' into implement-replicaof-failover-commands

cef6877

Signed-off-by: prateek-kumar-improving <prateek.kumar@improving.com>

Remove pause/unpause duplicate code

43285a7

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Remove pause/unpause duplicate code

e609282

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Refactor methods in valkey_glide_commands.c

5748310

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Refactor tests

c814940

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Fix tests

1925210

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

Merge branch 'main' into implement-replicaof-failover-commands

62645b7

Signed-off-by: prateek-kumar-improving <prateek.kumar@improving.com>

Fix tests

49822f9

Signed-off-by: Prateek Kumar <prateek.kumar@improving.com>

nderraugh reviewed Jul 31, 2026

View reviewed changes

currantw reviewed Jul 31, 2026

View reviewed changes

-            if (!z_host || Z_TYPE_P(z_host) != IS_STRING || !z_port) {
+            if (!z_host || Z_TYPE_P(z_host) != IS_STRING || !z_port) {
+    VALKEY_LOG_ERROR("command_validation", "'to' array must contain string 'host' and 'port'");
+    zend_throw_exception(get_valkey_glide_exception_ce(),
+                         "'to' array must contain string 'host' and 'port'", 0);

	* @param array\|null $to Target replica as ['host' => string, 'port' => int], or null for auto-selection.
	* @param array\|null $to Target replica as ['host' => string, 'port' => int], or null for auto-selection.

		$info = $client->info('REPLICATION');
		$this->assertEquals('master', $info['role']);

Uh oh!

Conversation

prateek-kumar-improving commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Issue link

Features / Behaviour Changes

Implementation

Limitations

Testing

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nderraugh Jul 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

currantw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

prateek-kumar-improving commented Jul 27, 2026 •

edited

Loading

nderraugh Jul 31, 2026 •

edited

Loading