Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion adapter/redis_peer_limiter.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import (

const (
redisPerPeerLimitEnv = "ELASTICKV_REDIS_PER_PEER_CONNECTIONS"
defaultRedisPerPeerConnectionCap = 8
defaultRedisPerPeerConnectionCap = 64

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Keep the default peer cap below noisy-client levels

With this default, the per-peer limiter no longer constrains the noisy-client shape this limiter was added for: one host can open 37 heavy-command connections and all are accepted until 64, while the heavy-command worker pool is global (2*GOMAXPROCS) rather than per-peer. On production-sized nodes with fewer than 32 Ps, that single peer can again occupy every heavy-command slot and starve other clients unless operators override ELASTICKV_REDIS_PER_PEER_CONNECTIONS, so the raised cap should be scoped to proxy deployments or kept below the noisy-client range.

Useful? React with 👍 / 👎.

redisPeerLimitError = "ERR max connections per client exceeded"
unknownRedisPeer = "unknown"
)
Expand Down
6 changes: 3 additions & 3 deletions adapter/redis_peer_limiter_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ func TestRedisLeaderClientPoolStaysBelowPeerLimit(t *testing.T) {
}

func TestRedisLeaderClientPoolUsesSmallDefault(t *testing.T) {
server := NewRedisServer(nil, "", nil, nil, nil, nil, WithRedisPerPeerConnectionLimit(8))
server := NewRedisServer(nil, "", nil, nil, nil, nil, WithRedisPerPeerConnectionLimit(defaultRedisPerPeerConnectionCap))
client := server.getOrCreateLeaderClient("127.0.0.1:6379")
defer client.Close()

Expand All @@ -95,7 +95,7 @@ func TestRedisLeaderClientPoolsSharePeerBudget(t *testing.T) {
}{
{name: "low cap", limit: 2, wantNormal: 1, wantBlocking: 1},
{name: "four cap", limit: 4, wantNormal: 2, wantBlocking: 2},
{name: "default cap", limit: 8, wantNormal: 4, wantBlocking: 4},
{name: "default cap", limit: defaultRedisPerPeerConnectionCap, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize},
Comment on lines +106 to +107

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win

「legacy cap」の期待値を上限8に合わせてください。

limit: 8 のケースでデフォルトのプールサイズを期待しているため、プール合計が8を超える構成では Line 113 の上限検証に必ず失敗します。旧上限の回帰ケースは通常4・ブロッキング4(または上限から算出した値)を期待し、現在のデフォルト値の検証は別ケースに分離してください。

修正例
-		{name: "legacy cap", limit: 8, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize},
+		{name: "legacy cap", limit: 8, wantNormal: 4, wantBlocking: 4},
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
{name: "legacy cap", limit: 8, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize},
{name: "default cap", limit: defaultRedisPerPeerConnectionCap, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize},
{name: "legacy cap", limit: 8, wantNormal: 4, wantBlocking: 4},
{name: "default cap", limit: defaultRedisPerPeerConnectionCap, wantNormal: defaultRedisLeaderClientPoolSize, wantBlocking: defaultRedisBlockingLeaderClientPoolSize},
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@adapter/redis_peer_limiter_test.go` around lines 106 - 107, Update the
“legacy cap” case in the pool-size test to expect normal and blocking
allocations totaling no more than the limit of 8, typically 4 each or values
derived from that cap. Keep “default cap” as the separate case validating the
current default pool sizes.

} {
t.Run(tc.name, func(t *testing.T) {
server := NewRedisServer(nil, "", nil, nil, nil, nil, WithRedisPerPeerConnectionLimit(tc.limit))
Expand All @@ -107,7 +107,7 @@ func TestRedisLeaderClientPoolsSharePeerBudget(t *testing.T) {
}

func TestRedisBlockingLeaderClientUsesDedicatedBudgetedPool(t *testing.T) {
server := NewRedisServer(nil, "", nil, nil, nil, nil, WithRedisPerPeerConnectionLimit(8))
server := NewRedisServer(nil, "", nil, nil, nil, nil, WithRedisPerPeerConnectionLimit(defaultRedisPerPeerConnectionCap))
shared := server.getOrCreateLeaderClient("127.0.0.1:6379")
defer shared.Close()
blocking := server.getOrCreateBlockingLeaderClient("127.0.0.1:6379")
Expand Down
8 changes: 4 additions & 4 deletions cmd/redis-proxy/main_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,10 +72,10 @@ func TestDeriveSecondaryConcurrency(t *testing.T) {
name: "dual write derives from ElasticKV pool",
mode: proxy.ModeDualWrite,
primaryPoolSize: 128,
elasticKVPoolSize: 4,
wantWriteConcurrency: 2,
wantScriptConcurrency: 1,
wantBlockingConcurrency: 2,
elasticKVPoolSize: 16,
wantWriteConcurrency: 8,
wantScriptConcurrency: 4,
wantBlockingConcurrency: 8,
},
{
name: "shadow mode derives from ElasticKV pool",
Expand Down
4 changes: 2 additions & 2 deletions deploy/redis-proxy/docker-compose.ha.yml
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ services:
- -listen=:6379
- -primary=${REDIS_PROXY_PRIMARY:-redis:6379}
- -secondary=${REDIS_PROXY_SECONDARY:-elastickv:6380}
- -elastickv-pool-size=${REDIS_PROXY_ELASTICKV_POOL_SIZE:-4}
- -elastickv-pool-size=${REDIS_PROXY_ELASTICKV_POOL_SIZE:-16}
- -mode=${REDIS_PROXY_MODE:-dual-write-shadow}
- -metrics=:9191
networks:
Expand All @@ -46,7 +46,7 @@ services:
- -listen=:6379
- -primary=${REDIS_PROXY_PRIMARY:-redis:6379}
- -secondary=${REDIS_PROXY_SECONDARY:-elastickv:6380}
- -elastickv-pool-size=${REDIS_PROXY_ELASTICKV_POOL_SIZE:-4}
- -elastickv-pool-size=${REDIS_PROXY_ELASTICKV_POOL_SIZE:-16}
- -mode=${REDIS_PROXY_MODE:-dual-write-shadow}
- -metrics=:9191
networks:
Expand Down
4 changes: 2 additions & 2 deletions docs/design/2026_04_24_implemented_workload_isolation.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ Implementation status:
gated `EVAL`/`EVALSHA` path.
- Shipped: Layer 3 per-peer Redis connection admission in
`adapter/redis_peer_limiter.go`, wired through `RedisServer.Run` accept and
close hooks. Default cap is 8 per peer IP and is configurable via
close hooks. Default cap is 64 per peer IP and is configurable via
`ELASTICKV_REDIS_PER_PEER_CONNECTIONS` /
`WithRedisPerPeerConnectionLimit`. Redis leader-proxy clients use a small
explicit go-redis pool below that default cap, and Pub/Sub detached sockets
Expand Down Expand Up @@ -374,7 +374,7 @@ one check per accept, not per command.

### Recommended v1 shape

**Per-peer-IP connection cap, default `N=8`, env-configurable,
**Per-peer-IP connection cap, default `N=64`, env-configurable,

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P3 Badge Correct the documented per-peer default

The implementation still sets defaultRedisPerPeerConnectionCap to 8, and the new limiter test in this commit also asserts the default is 8, so documenting the implemented v1 shape as default N=64 makes the design doc contradict the actual server behavior. Operators who rely on this line without setting ELASTICKV_REDIS_PER_PEER_CONNECTIONS would still have connections from the same proxy peer rejected after 8 sockets, including a 16-connection proxy pool.

Useful? React with 👍 / 👎.

enforced at accept.** On reject, accept the TCP connection, write a
`-ERR max connections per client exceeded` RESP error, then close —
so the client sees a protocol-level message instead of a bare
Expand Down
20 changes: 10 additions & 10 deletions docs/redis-proxy-deployment.md
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ go build -o redis-proxy ./cmd/redis-proxy/
| `-secondary-db` | `0` | Secondary Redis DB number |
| `-secondary-password` | (empty) | Secondary Redis password |
| `-primary-pool-size` | `128` | Primary Redis backend connection pool size |
| `-elastickv-pool-size` | `4` | ElasticKV backend connection pool size |
| `-elastickv-pool-size` | `16` | ElasticKV backend connection pool size |
| `-secondary-write-concurrency` | `0` | Shared maximum for all asynchronous secondary writes, including scripts. `0` derives half of the secondary backend pool size, minimum `1` |
| `-secondary-script-concurrency` | `0` | Lua-script sublimit within `-secondary-write-concurrency`. `0` derives half of the shared write limit, minimum `1` |
| `-secondary-write-queue-size` | `0` | Bounded queue for non-script secondary writes. `0` derives `64 * concurrency`, clamped to `64..8192` |
Expand Down Expand Up @@ -94,9 +94,9 @@ docker run --rm \
-primary redis.internal:6379 \
-primary-password "${REDIS_PASSWORD}" \
-secondary elastickv.internal:6380 \
-elastickv-pool-size 4 \
-secondary-write-concurrency 2 \
-secondary-script-concurrency 1 \
-elastickv-pool-size 16 \
-secondary-write-concurrency 8 \

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Leave capacity for blocking-command replay

In dual-write/shadow deployments that copy this example, -secondary-write-concurrency now consumes the entire ElasticKV pool. The runtime derivation sets blocking replay concurrency to poolSize - writeConcurrency, and goBlockingReplay returns immediately when that capacity is 0, so mutating blocking commands such as BZPOPMIN/BLPOP will not be replayed to ElasticKV. Please keep the write limit below the pool size or set an explicit -secondary-blocking-replay-concurrency when documenting this migration mode.

Useful? React with 👍 / 👎.

-secondary-script-concurrency 4 \
-mode dual-write-shadow \
-secondary-timeout 5s \
-shadow-timeout 3s \
Expand All @@ -118,9 +118,9 @@ services:
- -listen=:6479
- -primary=redis:6379
- -secondary=elastickv:6380
- -elastickv-pool-size=4
- -secondary-write-concurrency=2
- -secondary-script-concurrency=1
- -elastickv-pool-size=16
- -secondary-write-concurrency=8
- -secondary-script-concurrency=4
- -mode=dual-write-shadow
- -metrics=:9191
depends_on:
Expand Down Expand Up @@ -213,7 +213,7 @@ Override backend wiring via env vars before `docker compose up`:
```bash
REDIS_PROXY_PRIMARY=redis.prod.internal:6379 \
REDIS_PROXY_SECONDARY=elastickv-1.prod.internal:6380,elastickv-2.prod.internal:6380,elastickv-3.prod.internal:6380 \
REDIS_PROXY_ELASTICKV_POOL_SIZE=4 \
REDIS_PROXY_ELASTICKV_POOL_SIZE=16 \
REDIS_PROXY_MODE=dual-write-shadow \
docker compose -f docker-compose.ha.yml up -d
```
Expand Down Expand Up @@ -414,7 +414,7 @@ groups:
| Parameter | Value | Description |
|-----------|-------|-------------|
| Redis connection pool size | 128 | Default go-redis pool size for Redis |
| ElasticKV connection pool size | 4 | Default per-leader pool; keep within the server per-peer connection limit |
| ElasticKV connection pool size | 16 | Default per-leader pool; keep within the server per-peer connection limit |
| Dial timeout | 5s | Backend connection timeout |
| Read timeout | 3s | Backend read timeout |
| Write timeout | 3s | Backend write timeout |
Expand All @@ -439,7 +439,7 @@ Recommended shutdown order: `redis-proxy -> application -> Redis / ElasticKV`.
### Secondary writes are falling behind
- Check `proxy_async_queue_depth`, `proxy_async_queue_delay_seconds`, and `proxy_async_drops_by_queue_total` before increasing concurrency.
- Check `proxy_backend_pool_pending_requests` and the `waits`/`timeouts` pool events. Pool waits mean concurrency is too high for the configured pool.
- Increase the ElasticKV pool only together with `ELASTICKV_REDIS_PER_PEER_CONNECTIONS`; keep `-secondary-write-concurrency` at or below the pool size.
- Increase the ElasticKV pool only together with `ELASTICKV_REDIS_PER_PEER_CONNECTIONS` when the proxy pool can exceed the server's per-peer cap; keep `-secondary-write-concurrency` at or below the pool size.
- A sustained `expired` rate means secondary throughput is below ingress. Increasing queue size only delays the loss; profile ElasticKV before raising concurrency.

### High divergence count
Expand Down
2 changes: 1 addition & 1 deletion proxy/backend.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (

const (
defaultPoolSize = 128
defaultElasticKVPoolSize = 4
defaultElasticKVPoolSize = 16

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Guard proxy default against old per-peer caps

When the redis-proxy image is rolled before ElasticKV nodes that still run the previous 8-connection per-peer default, this new default pool can open 16 leader connections from the same proxy peer. Those older servers reject connections above 8 with ERR max connections per client exceeded, so secondary concurrency above the old cap turns into write errors during a mixed-version rollout unless the server cap is raised first or the proxy default stays within the old limit.

Useful? React with 👍 / 👎.

defaultDialTimeout = 5 * time.Second
defaultReadTimeout = 3 * time.Second
defaultWriteTimeout = 3 * time.Second
Expand Down
2 changes: 1 addition & 1 deletion proxy/dualwrite.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ const (
// producer-before-consumer ordering settle; a bounded no-effect window keeps
// BZPOP load from turning into a long retry backlog.
blockingReplayInitialDelay = 250 * time.Millisecond
blockingReplayNoEffectRetryWindow = 500 * time.Millisecond
blockingReplayNoEffectRetryWindow = 100 * time.Millisecond

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Restore enough BZPOP replay retry budget

When a client does ZADD followed quickly by BZPOPMIN and the ZADD secondary replay is still queued or committing for more than about 300ms, this 100ms window makes noEffectReplayRetryLimit equal 2, so the ZREM replay gives up after only two backoffs and records a permanent miss while the item can later appear on the secondary. The async queues still allow work to remain within the 5s SecondaryTimeout, so this narrows a previously recoverable 300–600ms ordering delay into a divergence for mutating blocking pops.

Useful? React with 👍 / 👎.

// compactedRetryInitialBackoff is the first delay before retrying a secondary
// command that failed with a compacted-read error.
compactedRetryInitialBackoff = 10 * time.Millisecond
Expand Down
4 changes: 2 additions & 2 deletions proxy/proxy_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -784,7 +784,7 @@ func TestDualWriter_Blocking_BZPopReplayShortTimeoutStillAttemptsZRem(t *testing

func TestNoEffectReplayRetryLimitIncludesJitterBudget(t *testing.T) {
limit := noEffectReplayRetryLimit(context.Background(), blockingReplayNoEffectRetryWindow)
assert.Positive(t, limit)
assert.Equal(t, 2, limit)

var spent time.Duration
backoff := compactedRetryInitialBackoff
Expand Down Expand Up @@ -1334,7 +1334,7 @@ func TestDefaultBackendOptions(t *testing.T) {

func TestDefaultElasticKVBackendOptions(t *testing.T) {
opts := DefaultElasticKVBackendOptions()
assert.Equal(t, 4, opts.PoolSize)
assert.Equal(t, 16, opts.PoolSize)
assert.Equal(t, 5*time.Second, opts.DialTimeout)
assert.Equal(t, 3*time.Second, opts.ReadTimeout)
assert.Equal(t, 3*time.Second, opts.WriteTimeout)
Expand Down
Loading