Repository navigation
Remove Redis Cluster support - #132
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Core connection replacement and concurrent reader recovery behavior warrant final human review despite comprehensive tests.
0 open findings
What changed in this PR
Removes Redis Cluster support while preserving standalone Redis compatibility and improving offline reader recovery.
Changes:
- Replaces Cluster routing with standalone command/reader clients and one-time mode detection.
- Makes reader registration and recovery connection-independent.
- Removes Cluster fixtures, CI coverage, and obsolete documentation.
| File | Description |
|---|---|
RedisConnection.hpp |
Implements standalone clients and Cluster rejection. |
RedisAdapter.cpp |
Simplifies bucketing, copying, and reader recovery. |
RedisAdapter.hpp |
Updates reader and standalone API documentation. |
connection_policy_test.cpp |
Tests cached mode detection. |
lifecycle_test.cpp |
Tests offline reader recovery. |
write_failure_test.py |
Extends proxy classification and reconnect checks. |
write_error_test.cpp |
Adds constructor and Cluster-refusal scenarios. |
test.cpp |
Tests key schema and cross-base utilities. |
recovery_test.cpp |
Removes Cluster recovery coverage. |
.github/workflows/test.yml |
Removes Cluster CI smoke test. |
scripts/run-test-redis-cluster.py |
Deletes private Cluster fixture. |
create-cluster |
Deletes legacy Cluster creation script. |
cluster-start.sh |
Deletes Cluster startup wrapper. |
README.md |
Documents standalone-only support. |
docs/api.md |
Updates connection and recovery behavior. |
docs/building.md |
Removes Cluster test instructions. |
docs/stream-subscriptions.md |
Documents standalone subscription behavior. |
docs/redis-adapter-implementation-spec.md |
Preserves the braced wire-key schema. |
CHANGELOG.md |
Records Cluster removal and compatibility impact. |
.github/ISSUE_TEMPLATE/bug.yml |
Requests supported connection details. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
bigsamich
left a comment
There was a problem hiding this comment.
Reviewed at 95792a3. Looks good. The removal is clean: no Cluster references remain outside the new detection path and docs, reader_token() no longer depends on the connection, and CI (build-test, ASan, TSan) is green. Two minor items and a note:
-
A cached Cluster refusal still reconnects on every attempt (
RedisConnection::connect()). After_initial_server == Unsupported,connect()still builds a pool and sendsPINGbeforeacceptStandalone()returns false. On a Cluster endpoint, every failed write or read callsreconnect(0). The adapter keeps opening a fresh connection, as often as every ~100 ms, for the life of the process, and only the first attempt logs. Suggest returning false at the top ofconnect()once the cached result isUnsupported. Thecluster-refusalproxy test'sping_probes == 2would then become 1. -
The mode is cached per
RedisConnectionobject, not per endpoint.connect(opts)is public and can target a different host or socket; the newOfflineInitialConnectionDefersModeProbeUntilReachabletest does exactly that. The result from the first reachable endpoint then applies to every later one.RedisAdapteralways reconnects to_options.cxn, so the adapter is not affected. Either document this onconnect()or key the cache on host/port/path. -
Note: the PR drops the stop/restart-all-readers block from the reconnect thread. That also closes the "late reconnect thread restarts readers after
~RedisAdapterstopped them" path in #118 (part 2), which may be worth a line in the PR body. #118 stays open for readers restarted from callbacks viaremoveReader()andsetDeferReaders(false).
|
Thanks for the review. I classified both requested changes as non-blocking follow-ups and filed them without changing the approved PR:
The reconnect-thread reader-restart note is now recorded on #118. PR #132 removes that specific restart path, while #118 remains open for the callback-driven shutdown races. All required checks on this PR are green, and there are no open inline review threads. |
Summary
CLUSTER INFO, reject detected Cluster mode, and never repeat detection during reconnects{baseKey}:subKey, timestamps, payloads, standalone TCP/Unix sockets, authentication, and connection poolingCompatibility
RedisConnection::keyslot()method andcopy() == -2resultRedisAdapter,RA_Options,ping(key), andtime(key)source surfaces otherwise unchangedValidation
git diff --checkThe local Redis 7.0.12 fixture cannot run the existing
HEXPIREwatchdog case; the pinned Redis 7.4 CI fixture covers it.Review requested from @wsulli and @bigsamich.
Closes #65