You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@kvrocks.apache.org by "Asthenia0412 (via GitHub)" <gi...@apache.org> on 2026/10/15 21:24:32 UTC
[PR] feat(cluster): implement CLUSTER FAILOVER [FORCE|TAKEOVER] [kvrocks]
Asthenia0412 opened a new pull request, #3681:
URL: https://github.com/apache/kvrocks/pull/3681
## Background
Redis Cluster provides `CLUSTER FAILOVER [FORCE|TAKEOVER]` to let operators promote a replica to master. This is essential for planned maintenance or when the current master is unreachable.
Apache Kvrocks currently rejects this command, so the only recovery path when a master gets stuck is to delete and recreate the entire cluster via the controller — disruptive even when a fully synced replica is available.
## Problem
```
> CLUSTER FAILOVER FORCE
-ERR CLUSTER command, CLUSTER INFO|NODES|SLOTS|KEYSLOT|RESET|REPLICAS
```
Neither `CLUSTER` nor `CLUSTERX` namespace implements failover.
## Solution
Implement Redis-compatible `CLUSTER FAILOVER [FORCE|TAKEOVER]`. This PR focuses on **TAKEOVER** mode (operator-driven, master known to be unreachable):
### Changes
**`src/cluster/cluster.h`** — Added `Failover(bool takeover)` method declaration.
**`src/cluster/cluster.cc`** — Core failover logic:
1. Validate current node is a slave with a known master in cluster topology
2. Promote self to master: `role = kClusterMaster`, clear `master_id`
3. Demote old master to slave: `role = kClusterSlave`, `master_id = self_id`
4. Increment `version_` (config epoch) and recalculate master count
5. Clear migrated/imported slot info
6. Reuse existing `SetMasterSlaveRepl()` to switch replication thread
**`src/commands/cmd_cluster.cc`** — Command parsing + execution:
- `CLUSTER FAILOVER` (defaults to TAKEOVER)
- `CLUSTER FAILOVER TAKEOVER`
- `CLUSTER FAILOVER FORCE` (falls back to TAKEOVER; true FORCE coordination is a follow-up)
- Persist cluster nodes info after failover when `persist_cluster_nodes_enabled` is true
### FORCE vs TAKEOVER
For the initial implementation, FORCE is treated identically to TAKEOVER. A full FORCE mode (coordinating with the living master for a graceful handoff) requires cross-node RPC plumbing that is outside the scope of this PR — but the command syntax is accepted and a TODO comment marks where that logic should go.
### Design choices
- **Reuse `SetMasterSlaveRepl()`** — This is the same code path used by cluster topology updates via `CLUSTERX SETNODES`, ensuring failover goes through the same master-removal / master-addition logic, including stopping/restarting the slot migrator.
- **Config epoch increment** — Following Redis Cluster semantics, the new topology gets a bumped version so other nodes pick it up via gossip.
- **No slot redistribution** — Slots stay with the promoted master (they already belong to it as a replica of the old master). The old master, now demoted, keeps its slots too — it will sync from the new master.
## Testing
- Manual compilation: `make` (in progress)
- Manual integration test: TBD (will be added after CI validation)
- No new unit tests added yet — following the pattern of existing `Cluster` methods that rely on integration coverage
## Follow-ups
- Full FORCE mode implementation (coordinate with living master before TAKEOVER)
- `CLUSTER FAILOVER` timeout / delay support
- Integration test in `tests/go/cluster_test.go`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cluster): implement CLUSTER FAILOVER [FORCE|TAKEOVER] [kvrocks]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3681:
URL: https://github.com/apache/kvrocks/pull/3681#issuecomment-6202601275
Hi @Asthenia0412,
Thank you for your pull request. Please review our [Contributing Guide](https://kvrocks.apache.org/community/contributing/).
Please make sure you understand your changes and explain your reasoning in this pull request. Low-quality pull requests may be closed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] feat(cluster): implement CLUSTER FAILOVER [FORCE|TAKEOVER] [kvrocks]
Posted by "weim0000 (via GitHub)" <gi...@apache.org>.
weim0000 commented on PR #3681:
URL: https://github.com/apache/kvrocks/pull/3681#issuecomment-6206748114
Hi @Asthenia0412 , thanks for your contribution and interest in improving Kvrocks!
I'd like to raise a design concern about adding CLUSTER FAILOVER to kvrocks-server.
Unlike Redis Cluster which uses a decentralized gossip-based failover (replica self-election via majority vote), Kvrocks adopts a centralized cluster management model. The cluster topology — including failover, slot migration, and scaling — is managed externally by [kvrocks-controller](https://github.com/apache/kvrocks-controller), which pushes topology updates to all nodes via CLUSTERX SETNODES.
This design is explicitly stated in the codebase:
> "We use central controller to manage the topology of the cluster. Server can't change the topology directly."
> — src/cluster/cluster.cc, CanExecByMySelf()
And in the README:
> "To manage Kvrocks clusters for failover, scaling up/down and more, use kvrocks-controller"
Implementing CLUSTER FAILOVER on the server side would allow a replica to promote itself without the controller's coordination, which could lead to:
- Topology inconsistency between the controller's view and the actual server state
- Split-brain risk if the controller simultaneously pushes a different topology
- Architectural conflict with the centralized management model that Kvrocks deliberately chose
If there's a specific use case you're trying to address (e.g., manual failover without a controller, or a simpler deployment scenario), could you share more context? There might be a better approach that aligns with the existing architecture — for instance, adding a failover API to kvrocks-controller, or supporting a CLUSTERX subcommand that requests the controller to initiate a failover rather than executing it locally.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@kvrocks.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org