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:35:32 UTC
[PR] feat(cluster): add master lease for split-brain prevention (Phase 1) [kvrocks]
Asthenia0412 opened a new pull request, #3682:
URL: https://github.com/apache/kvrocks/pull/3682
## Background
During a network partition, the old kvrocks master may continue accepting writes
indefinitely because it has no way to know the controller has already promoted a
replica to be the new master. This creates split-brain — two masters coexist and
data diverges.
Controller failover currently works like this:
1. Partition happens -> old master M is isolated, unaware
2. After ping_interval * max_ping_count failures (e.g. 15s), controller promotes replica S
3. M never receives the SETNODES; it still believes itself to be master and keeps writing
4. When the partition heals, M is demoted and all writes during the partition are lost
## Problem
No mechanism exists to gate master writes on liveness. A partitioned master keeps
accepting writes until it receives a new topology — which may never arrive.
## Solution
Implement Phase 1: local-cache lease (per the simplified design proposed by
maintainer jihuayu in issue #3380):
Controller push lease (SETNODES) -> kvrocks master caches expire_at locally
Per-write check
if now > expire_at -> REJECT writes
This is a pragmatic cost/benefit tradeoff:
- No per-write network round-trip
- Max split-brain window shrinks from ~21s to ~12s
- Configurable and opt-in — zero overhead when disabled
### Changes
**`src/cluster/cluster.h`**:
- `master_lease_expire_at_ms_` (int64_t) — epoch ms when lease expires, 0 = unset
- `master_lease_duration_sec_` (int64_t) — config-driven duration, 0 = disabled
- `RefreshMasterLease()`, `IsMasterLeaseExpired()`, `IsMasterLeaseEnabled()`, `SetMasterLeaseDuration()`
**`src/cluster/cluster.cc`**:
- `RefreshMasterLease()`: extends expire_at by duration
- `IsMasterLeaseExpired()`: compares GetTimeStampMS() vs cached expire_at
- Hooked into SetClusterNodes() success path — every controller topology update refreshes lease
- Injected lease-expiry check into CanExecByMySelf() write path — master rejects writes with CLUSTERDOWN when expired
### Design choices
- Push from controller, not pull from kvrocks — jihuayu's recommendation
- Opt-in via master_lease_duration_sec_ > 0 — zero behavioral change when disabled
- Fail-open on bootstrap: if lease never set up (expire_at == 0), writes proceed normally
## Testing
- Manual compilation: make (in progress)
- Integration test: TBD
Reference: https://github.com/apache/kvrocks/issues/3380
--
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): add master lease for split-brain prevention (Phase 1) [kvrocks]
Posted by "jihuayu (via GitHub)" <gi...@apache.org>.
jihuayu commented on PR #3682:
URL: https://github.com/apache/kvrocks/pull/3682#issuecomment-6291303637
@Asthenia0412 Could you please add an AI-use disclosure to the PR description, following the [contribution guidelines](https://github.com/apache/kvrocks-website/blob/main/community/contributing.md#guidelines-for-ai-assisted-contributions)? Please state whether AI was used and, if so, which tools were used, what they helped with, and any parts needing extra review. Thanks!
--
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): add master lease for split-brain prevention (Phase 1) [kvrocks]
Posted by "weim0000 (via GitHub)" <gi...@apache.org>.
weim0000 commented on PR #3682:
URL: https://github.com/apache/kvrocks/pull/3682#issuecomment-6206970503
Hi @Asthenia0412, thanks for looking into the split-brain problem described in #3380 — this is indeed a real issue worth addressing, and the opt-in, local-cache approach is a good starting point.
I have a few suggestions that would help strengthen the implementation:
- Configuration wiring: SetMasterLeaseDuration() is provided but doesn't appear to be connected to any config entry or CONFIG SET command yet. It would need a config option (e.g., master-lease-duration-sec) for users to actually enable and control this feature.
- Master ID / version validation: The issue design in #3380 mentions that "the controller probe must carry current master ID, cluster version, and lease duration" and the lease should only extend when they match. It would be important to add this validation in SetClusterNodes() to ensure the lease is only refreshed for the intended master, not on every topology update.
- Scope of RefreshMasterLease(): Currently this is called for all nodes in SetClusterNodes(). Since only the master needs to gate writes on the lease, it might be cleaner to restrict this to the master path only.
- Configurable behavior on lease expiry: Right now an expired lease always rejects writes with CLUSTERDOWN. However, different deployments may have different preferences — some users might prefer stricter consistency (reject writes), while others might prefer higher availability (keep serving and let the controller reconcile eventually). A config option like master-lease-expiry-policy with values like reject-writes / keep-serving could let users make that choice based on their scenario.
One minor note: this PR bundles two features (CLUSTER FAILOVER and master lease) — it might be cleaner to split them into separate PRs since they address different concerns.
Would you be open to refining the master lease work with these suggestions? Happy to discuss further if anything is unclear. Thanks again for the contribution!
--
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): add master lease for split-brain prevention (Phase 1) [kvrocks]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #3682:
URL: https://github.com/apache/kvrocks/pull/3682#issuecomment-6202781808
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