Repository navigation
[Store] HA optlog support redis backed - #4295
Conversation
github-actions
Bot
added
documentation
|
Thanks for working on Redis-backed OpLog support. The overall direction makes sense: it removes the extra etcd dependency for deployments that already use Redis for HA leadership, while reusing the existing HaKvBackend contract. I have a few concerns that I think should be discussed before merging.
The feature itself is useful, but I would prefer to settle the durability model and recovery behavior before merging. Once those semantics are explicit and covered by tests, the Redis-backed OpLog design should be much easier to review and operate safely. |
|
@Icedcoco about first question,my thinking is below: Thanks for raising this. I agree that the current wording is too ambiguous about what a successful OpLog write/transaction guarantees. The key point is that appendfsync everysec should not be presented as providing strict durability for acknowledged OpLog batches. A partial loss of an acknowledged batch could indeed leave the standby with an incomplete OpLog and make promotion unsafe. For the current implementation, I propose to explicitly define Redis as an ephemeral OpLog/coordination store, rather than claiming strict durability. In particular: A successful Redis transaction means that Redis accepted the write, but does not imply durable commitment. We will not rely on Redis persistence for safe promotion. If the OpLog namespace is missing, empty, or fails validation after recovery, the system will treat the Redis state as lost and rebuild the cluster state from the authoritative source. appendfsync everysec can therefore remain the default operational setting, since the correctness model does not depend on recovering acknowledged OpLog entries from Redis. I agree that if we want to support promotion from the Redis/standby OpLog without a full rebuild, we would need a stronger contract: explicit commit semantics, replication guarantees, and a rule that an acknowledged batch is fully recoverable. I will update the documentation to make this distinction explicit and avoid implying that appendfsync everysec provides strict OpLog durability. |
|
@Icedcoco Thanks for the detailed review. I have addressed the issues raised above.
Thanks again for pointing these out. The unrelated repository changes and the incorrect Cluster support description have been fixed, and the HA recovery behavior is now covered by focused tests. |
Removing hot_standby_snapshot_bootstrap_test.cpp also dropped every StandbyControllerTest case, including the Redis promotion gate that kvcache-ai#4295 added. Move the cases that do not depend on the legacy catalog snapshot path into standby_controller_test.cpp, and point the real-etcd snapshot smoke back at the lifecycle case. Cases that configure the removed enable_snapshot_restore and snapshot_catalog_store_type options stay deleted.
Mainline kvcache-ai#4295 removed this include block; a conflict resolution kept it. No symbol from either header is used in this file.
Description
A Redis HA cluster could already elect a leader, but the batch OpLog could not follow it. OrderedOpLogWriter and the standby reader talk to a HaKvBackend: one compare-and-swap transaction per batch, then a lexicographic range to catch up. Only the etcd backend implemented that contract, so --enable_oplog=true with ha_backend_type=redis was rejected at startup, and a cluster that already ran Redis still needed a second etcd just to store acknowledged batches. Leadership and the log are separate; this change gives the log a Redis implementation and leaves snapshots on etcd.
What changes
RedisHaKvBackend implements the existing KV contract. The writer, the standby reader, and the applier are unchanged. CreateHaKvBackend is the only place that opens the backend, and the master writer, ClaimProducerView, and the standby follower all go through it. etcd still takes the etcd branch. k8s is rejected. A build without STORE_USE_REDIS or STORE_USE_ETCD returns UNAVAILABLE_IN_CURRENT_MODE from the factory, so the no-backend stub that aborts the process is never called.
Logical keys stay /oplog/{cluster}/producer_view, durable_prefix, and batches/{id}. Redis stores the bytes at mooncake-store/{cluster}/oplog/... and orders those logical keys in the ZSET mooncake-store/{tag}/oplog-index. The hash tag puts the index and the records on one slot, so the Lua script is valid on Redis Cluster. The connection is the backend's own; it is not the connection the leader renew thread uses.
One acknowledged batch is one EVAL. The script checks that producer_view equals the claimed view and durable_prefix equals the expected prefix, then writes the batch record and advances the prefix. A failed compare returns ETCD_TRANSACTION_FAIL. A reply that never comes back resets the connection and returns ETCD_OPERATION_ERROR; the existing read-back path treats that as ambiguous and reconciles. kCreateRevisionEquals returns INVALID_PARAMS before any command is sent. Redis has no create revision, and the snapshot maintenance lease still needs one.
Splitting the log from the snapshot leaves two races the commit of a batch already had, and the Redis mapping has to preserve them:
A range or a delete is [begin, end). limit 0 means the whole interval; a Redis COUNT of 0 would return nothing. The next page starts at last_key + '\0'. Bounds that are empty, a single NUL, or begin > end are rejected, and one transaction may not touch two cluster ids.
A dropped connection can hide a script that did commit. The backend does not retry that call. The caller reads the prefix back and continues from what Redis actually stored.
Behavior change: --enable_oplog on Redis. With STORE_USE_REDIS and a connstring, ha_backend_type=redis now creates the batch writer and the standby follows the same Redis. Before, that combination fataled, and an empty connstring still skips the writer. enable_oplog_snapshot and snapshot restore still require ha_backend_type=etcd. Redis has to run AOF with appendfsync everysec or always; a restart without it can drop batches the master has already acknowledged.
Smaller changes:
HaBackendSupportsOpLog is one switch on HABackendType. Adding a backend is that case plus a factory branch.
The shard scan copies buffer descriptors only for replicas on the segments being re-adopted. The Redis range copies values only for keys inside the requested interval.
NoF placeholder allocators stay out of this path. Nothing in the Redis backend adopts them.
The deployment guide states that the same Redis holds leader election and the batch log, and that clients still use the redis:// master address. The restore-side log is unchanged.
CreateRevisionCompareIsRejected always runs. The four integration tests run only when the binary is built with STORE_USE_REDIS and --redis_endpoint is set; gtest_main does not parse gflags, so this target supplies its own main.
Module
mooncake-transfer-engine)mooncake-store)mooncake-conductor)mooncake-reshard)mooncake-ep)mooncake-pg)mooncake-integration)mooncake-p2p-store)mooncake-wheel)mooncake-common)mooncake-rl)Type of Change
How Has This Been Tested?
Test commands:
redis-server --port 6379 --daemonize yes
./build/mooncake-store/tests/redis_ha_kv_backend_test
--redis_endpoint=127.0.0.1:6379
Test results:
Checklist
./scripts/code_format.shAI Assistance Disclosure
Codex assisted with the RedisHaKvBackend implementation (Lua CAS and ZSET range/delete), wiring CreateHaKvBackend into the master, standby, and supervisor, the Redis integration tests, and the deployment-guide notes for Redis OpLog. The test target now parses gflags itself so --redis_endpoint takes effect. The submitter has reviewed every changed line and is responsible for the change.