redis: add pub/sub support via RESP3 Push messages#46354
Closed
bellatoris wants to merge 4 commits into
Closed
Conversation
Add SUBSCRIBE/PSUBSCRIBE/SSUBSCRIBE forwarding in the redis_proxy filter and RESP3 Push-frame delivery back to subscribed downstream clients. Builds on the RESP3 init pipeline introduced in lyyvkozu. * port the SUBSCRIBE pipeline onto Client::sendCommand and the new PushMessageCallbacks plumbing * unify ClientImpl's held queue across user requests and fire-and-forget commands so SUBSCRIBE issued during HELLO 3 / READONLY / IAM token fetch is held and replayed in original submission order * sendCommand now respects both the init gate and queue_enabled_; replay uses absl::variant + absl::visit to dispatch each entry by type * onRespValue routes Push frames to push_callbacks_ when set, drops them with a debug log otherwise, and tears the connection down on a non-Push reply with empty pending_requests_ instead of asserting (Rule 5) * onEvent(Connected) no longer asserts on empty pending_requests_, so the AWS IAM pending-token case can connect cleanly * subscriptionRegistryShared() gates on the typed enum upstream protocol version, getOrCreateSubscriptionClient is just lookup + setPushCallbacks, and conn_pool_impl uses Common::Redis::Utility::makeRequest in place of the previously-undefined buildCommand helper * remove plumbing made redundant by the init-pipeline refactor: sendAwsIamAuth public API, queueRequests, makeRequestImmediate, Hello3Callbacks, hello3_sent_hosts_, the uint32 protocol_version_ field, and the downstreamRespVersion / upstreamProtocolVersion / setExplicitHelloVersion / explicitHelloVersion accessors * add 6 focused tests covering sendCommand held during AwaitingHello / AwaitingReadonly / WaitingForAwsToken, mixed user+sendCommand FIFO replay, Push delivery with empty pending, and non-Push reply close Signed-off-by: Doogie Min <doogie.min@sendbird.com>
…Y fixed, behavior-invariant) — recordedOwnerValid/resolvePlacement helpers, reissueSsubscribe + onClusterTopologyChange rewritten to 'record valid → retarget / invalid → re-place', synthetic sendUpstreamSsubscribe removed (fresh subscribe → resolvePlacement+sendToHost+record). All existing suites green. Signed-off-by: Doogie Min <doogie.min@sendbird.com>
Round-5 must-fix/recommended (prior): H1/H2 (C-1 DNS-Loading regression: pop-cascade hang + cancel UAF, one in_dns_redirect_ state flag), H3 doc (health-escape depends on Host health flipping), M1 (non-cluster validity degrades to PRIMARY), M2 (backoff coalesce vs stale pool timer), M3 (CLUSTER SLOTS range hardening), M4 (ValidityMode enum), M5 (whole-shard-unhealthy anti-ping-pong), A2 (drop speculative as_primary/ShardMembers::primary), A3/A4 helpers, §3.1/§3.2.
Deferred batch (this pass, all Docker-green):
- §5 efficiency: cache ShardMembershipResolver (dyn_cast once per cluster update), zero-copy membersForSlot, reuse validity candidates for SHARD_MEMBERS re-placement (E-8 parity), allocation-free reservoir tie-break in leastLoadedOf, addSubscription returns {subscriber_newly_added, first_subscriber} (one hash), absl::c_linear_search for membership scans, resetResubscribeCycle() helper, relocate RedirectionValues to client.h and reuse MOVED/ASK/CLUSTERDOWN in the registry.
- §3.3: trim stale "former X was removed" comment narration (5 sites).
- A1: remove prod-dead onUpstreamConnectionClose (onEvent open-codes the 3 primitives for weak-capture posting); drop the null-host whole-registry branch + ASSERT non-null; port tests to a simulateUpstreamConnectionClose helper.
- M6: scope a -MOVED re-mark to the correlated channel (not the whole host) so a slot-migration pre-refresh window does not churn a host every backoff; whole-host fallback only for an uncorrelated (empty-FIFO) redirect; backoff+refresh remain the safety net.
Regression tests added; unit targets Docker-green.
Review follow-up: onClusterRemoval now drains pending_requests_ so a request parked mid DNS-redirect (in_dns_redirect_, attached to no client) is failed via ~PendingRequest instead of awaiting a DNS continuation for a gone cluster (finding 1, regression test). Redirect classifier matches the leading error-code TOKEN (code + space/end), not a bare prefix, so "MOVEDX ..." is not misread as -MOVED (finding 3, regression test).
Review follow-up 2: shardCandidatesForChannel health-tiers Healthy -> Degraded -> all (was Healthy -> all), matching Envoy LB semantics — a degraded-but-usable member is preferred over an unhealthy one, and the tiering also lets a failure-driven escape leave an unhealthy owner without waiting for the whole shard to go down (test extended with mixed/degraded/all-fallback cases). Also stripped all internal review-tracking labels (D1/S6-x/E-8/§N/Issue N/Finding N/#N/...) from the pub/sub stack's comments so upstream readers see self-contained rationale, not review-round shorthand; comment-only, unit targets Docker-green.
Review follow-up 4: (1) documented the pub/sub delivery-ordering caveat in redis.rst (an out-of-band unsubscribe ack can precede a message parked behind a pipelined command reply for the same channel — within best-effort delivery, but strict-state-machine clients should not pipeline commands with an in-flight UNSUBSCRIBE); (2) subscription_registry_lib include/deps hygiene — added direct includes (envoy/network/connection.h, source/common/buffer/buffer_impl.h) and matching BUILD deps (network/stats/upstream/common:buffer/common:random_generator) for types the header uses directly; (3) inclusive language — 'master' -> 'primary' in explanatory comments (MASTER enum literals unchanged); (4) dropped internal review-history prose ('round 3', 'F-series') from two comments.
Signed-off-by: Doogie Min <doogie.min@sendbird.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
bellatoris
force-pushed
the
doogie/resp3-pubsub
branch
5 times, most recently
from
July 24, 2026 02:31
711b090 to
66dd4f1
Compare
|
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
bellatoris
force-pushed
the
doogie/resp3-pubsub
branch
3 times, most recently
from
July 24, 2026 03:06
1229e17 to
56ec800
Compare
…ol acks + doc/marker fixes Signed-off-by: Doogie Min <doogie.min@sendbird.com>
bellatoris
force-pushed
the
doogie/resp3-pubsub
branch
from
July 24, 2026 13:46
56ec800 to
5a4b7ab
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Commit Message:
Additional Description:
Follow-up to #44936, which left
SUBSCRIBEforwarding as future work. Adds pub/sub toredis_proxyon RESP3 listeners, built on Redis 7 sharded pub/sub: downstreamSUBSCRIBE/UNSUBSCRIBEare rewritten upstream toSSUBSCRIBE/SUNSUBSCRIBE, so subscriptions route by cluster slot exactly like keys, and messages reach the client as RESP3 Push frames. A per-workerSubscriptionRegistryowns placement and healing: each channel has one owning upstream host on a dedicated subscription connection, subscribe attempts are generation-tagged and correlated to their out-of-band acks through a per-host FIFO ledger, and upstream connection loss,-MOVED/-ASKon control commands, failover, and slot migration all re-place and re-subscribe with backoff — invisible to the downstream client. Acks are delivered at their position in the request FIFO, pushes never interleave inside pipelined replies, and slow subscribers are bounded by watermark checks plus eviction (the proxy-side analogue ofclient-output-buffer-limit). Placement isPRIMARY(default) orSHARD_MEMBERS(least-loaded across the shard's primary and replicas);PUBLISHcan opt intoSPUBLISHrewriting viaenable_sharded_publish.Design note: sharded-only, deliberately. Classic broadcast pub/sub (
SUBSCRIBEfan-out to every node,PSUBSCRIBEpatterns) has no slot affinity, so a proxy in front of a cluster cannot place or heal it coherently.PSUBSCRIBE/PUBSUB/RESETremain rejected; the sharded verbs are not client-exposed (and the pub/sub names can no longer be declared incustom_commands);sharded_subscription_mode: DISABLEDrestores the pre-feature rejection for Redis 6.x upstreams. Separately, commands that fail to queue inside aMULTI(unknown, wrong arity, locally-answered, malformed frames, routing failures) now dirty the transaction soEXECaborts with-EXECABORT, matching real RedisCLIENT_DIRTY_EXECsemantics — this applies to all listeners and is changelogged.Risk Level: Medium. Pub/sub is opt-in (
protocol_version: RESP3plus actual subscribe traffic); the RESP2 default path is unchanged. The transaction dirty-EXECcorrections apply to all listeners and are listed in the changelog.Testing: Unit + integration suites (registry state machine incl. generation/FIFO/topology handling, conn-pool subscription connections, transaction×pubsub boundary, downstream ordering and watermarks). Additionally verified against a real Redis 7.2 cluster (3 primaries + 3 replicas), including subscribe continuity across failover and live slot resharding.
Docs Changes:
redis_proxy_filter.rst(pub/sub configuration + new stats), arch overview redis page, API proto docs.Release Notes:
changelogs/current/—new_features(pub/sub support),minor_behavior_changes(transaction dirty-EXEC),behavior_changes(pub/sub names rejected incustom_commands).Runtime guard: N/A — feature is config-gated.
Platform Specific Features: N/A
I used generative AI to help author this change.