Repository navigation
QoS for system critical events ( such as cluster heartbeats, replication streams, and slot migration) - #4076
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a secondary high-priority event loop linked into the main event loop, adds a connection priority ( ChangesSocket prioritization implementation
Estimated code review effort: 5 (Critical) | ~100 minutes Sequence Diagram(s)sequenceDiagram
participant MainThread
participant IOThread
participant HighInbox as io_shared_inbox[HIGH]
participant NormalInbox as io_shared_inbox[NORMAL]
MainThread->>HighInbox: enqueue read/write job (qidx=HIGH)
MainThread->>NormalInbox: enqueue read/write job (qidx=NORMAL)
IOThread->>HighInbox: drain bounded high-priority jobs
IOThread->>NormalInbox: process normal-priority jobs
IOThread->>MainThread: sendToMainThread via io_shared_outbox[qidx]
MainThread->>MainThread: processIOThreadsResponses drains high then normal
sequenceDiagram
participant Client
participant Server_el as server.el
participant Server_hp_el as server.hp_el
Client->>Server_el: connect (normal priority)
Server_el->>Server_el: register AE_READABLE/AE_WRITABLE
Client->>Server_el: setname sentinel-...
Server_el->>Server_hp_el: connSetPriority(CONN_PRIORITY_HIGH), migrate AE events
Server_el->>Server_hp_el: PROCESS_HP_EVENTS_IF_NEEDED during poll
Server_hp_el-->>Client: prioritized handling of events
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
c5bdcff to
86ff5b2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
src/tls.c (1)
1201-1201: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated priority-derivation logic.
The
ae_pri = (conn->c.priority > CONN_PRIORITY_NORMAL) ? AE_PRIORITY_HIGH : AE_PRIORITY_NORMAL;computation is repeated verbatim inregisterSSLEventandupdateSSLEvent. Extracting it into a small helper (e.g.connPriorityToAePriority(conn)) would avoid divergence if the mapping logic changes later.♻️ Proposed refactor
+static inline int connPriorityToAePriority(connection *conn) { + return (conn->c.priority > CONN_PRIORITY_NORMAL) ? AE_PRIORITY_HIGH : AE_PRIORITY_NORMAL; +} + static void registerSSLEvent(tls_connection *conn) { int mask = aeGetFileEvents(server.el, conn->c.fd); - int ae_pri = (conn->c.priority > CONN_PRIORITY_NORMAL) ? AE_PRIORITY_HIGH : AE_PRIORITY_NORMAL; + int ae_pri = connPriorityToAePriority((connection *)conn);static void updateSSLEvent(tls_connection *conn) { ... - int ae_pri = (conn->c.priority > CONN_PRIORITY_NORMAL) ? AE_PRIORITY_HIGH : AE_PRIORITY_NORMAL; + int ae_pri = connPriorityToAePriority((connection *)conn);Also applies to: 1253-1253
🤖 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 `@src/tls.c` at line 1201, The AE priority mapping is duplicated in both registerSSLEvent and updateSSLEvent, so extract the conn->c.priority to AE_PRIORITY_HIGH/AE_PRIORITY_NORMAL conversion into a small shared helper such as connPriorityToAePriority and use it from both call sites. Keep the helper near the existing SSL event logic in src/tls.c so the priority-derivation stays consistent and any future mapping change only needs one update.src/commands/client-list.json (1)
211-217: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSame documentation/metadata suggestions as
client-kill.json.This
QOSargument has the identical gap: nohistoryentry for the10.0.0addition, and it's typed as a genericstringrather thanoneofwith pure-tokens for the two fixed values (normal-priority/high-priority), unlike other fixed-value filters in this file (e.g.,client-type/TYPE,skipme/SKIPME).🤖 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 `@src/commands/client-list.json` around lines 211 - 217, The QOS argument metadata in client-list.json is missing the same documentation and schema updates as client-kill.json: add a history entry for the 10.0.0 introduction and change the definition from a generic string to a oneof with the two fixed pure-token values. Update the QOS entry in the client-list argument schema to match the pattern used by fixed-value filters like client-type/TYPE and skipme/SKIPME, using the relevant metadata fields on the QOS item.src/networking.c (1)
4912-4919: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a
not-qosnegative filter for consistency.Every other filter in this function (type, addr, laddr, user, name, flags, lib-name, lib-ver, db, capa, ip) has a corresponding
not-*negative counterpart, butqosonly supports positive matching. This is a minor completeness/consistency gap rather than a defect.Also applies to: 4998-5003
🤖 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 `@src/networking.c` around lines 4912 - 4919, The client-filter parsing in networking.c should add a negative counterpart for qos to match the existing not-* pattern used by type, addr, laddr, user, name, flags, lib-name, lib-ver, db, capa, and ip. Update the filter handling around the qos branch in the client filter parser (the same logic used in the nearby helper that processes other filter tokens) so that not-qos is recognized and mapped to the corresponding negated filter field, mirroring how the other positive/negative pairs are implemented.tests/unit/socket-prioritization.tcl (1)
20-83: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffTiming-based throttling test may be flaky under load, but is appropriately scoped.
This test relies on fixed sleeps (
after 50,after 1100) combined withDEBUG SLEEPto force event accumulation. It's already gated behindneeds:debug external:skip, which limits its blast radius to internal test runs, but such fixed-delay synchronization can still be flaky on slow/loaded CI runners (e.g., if the server doesn't finish accepting 250 connections or the sleep window is too tight).🤖 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 `@tests/unit/socket-prioritization.tcl` around lines 20 - 83, The socket prioritization test in the test body that uses DEBUG SLEEP, after 50, and after 1100 is timing-sensitive and can be flaky on slow runners; replace the fixed-delay synchronization with a more deterministic readiness check or polling loop before sending the 250 SET commands and before reading results. Keep the existing scope markers (needs:debug external:skip) and adjust the test around the existing client setup, control_cl flow, and telemetry assertions so it waits for the intended server state instead of assuming exact sleep timing.src/commands/client-kill.json (1)
229-235: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider documenting the QOS addition in
history, and modeling it asoneof.Two small metadata improvements for this new argument:
- Other additions in this file (e.g., the
9.0.0entry) get a top-levelhistoryentry describing the new filter; thisQOSaddition at10.0.0doesn't have one.QOSonly accepts two fixed values (normal-priority/high-priority), yet is typed as a genericstring. Other fixed-value filters here (client-type/TYPE,skipme/SKIPME) use"type": "oneof"withpure-tokensub-arguments, which better documents allowed values viaCOMMAND DOCS/COMMAND INFO. Since the actual parsing inparseClientFiltersOrReply(src/networking.c) is hand-written viastrcasecmp, this would be a metadata-only change with no functional impact.💡 Example history + oneof restructuring
[ "9.0.0", "Added filters NAME, IDLE, FLAGS, LIB-NAME, LIB-VER, DB, CAPA, and IP. And negative filters NOT-ID, NOT-TYPE, NOT-ADDR, NOT-LADDR, NOT-USER, NOT-FLAGS, NOT-NAME, NOT-LIB-NAME, NOT-LIB-VER, NOT-DB, NOT-CAPA, NOT-IP." + ], + [ + "10.0.0", + "Added the QOS filter option." ] ], ... { "token": "QOS", "name": "qos", - "type": "string", - "optional": true, - "since": "10.0.0" + "type": "oneof", + "optional": true, + "since": "10.0.0", + "arguments": [ + {"name": "normal-priority", "type": "pure-token", "token": "normal-priority"}, + {"name": "high-priority", "type": "pure-token", "token": "high-priority"} + ] },🤖 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 `@src/commands/client-kill.json` around lines 229 - 235, Add a top-level history entry for the new QOS filter in client-kill.json so it is documented alongside the other versioned additions, and change the QOS argument definition from a generic string to a oneof. Model the allowed values as pure-token sub-arguments for the two fixed options (normal-priority and high-priority), matching the existing client-type/TYPE and skipme/SKIPME patterns so COMMAND DOCS and COMMAND INFO expose the real choices. Keep this as metadata only; no changes are needed in parseClientFiltersOrReply.src/unit/test_socket_prioritization.cpp (1)
120-150: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd one unit case for listener accept-handler priority propagation.
The suite covers direct file-event priority and
connSetPriority(), but it does not hit the newcreateSocketAcceptHandler()/connListener::prioritypath insrc/server.c. A small test that registers a listener withCONN_PRIORITY_HIGHand inspects the createdaeFileEventwould close that gap. As per coding guidelines,src/unit/**/*.{c,cpp}low-level logic changes should be covered by unit tests.🤖 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 `@src/unit/test_socket_prioritization.cpp` around lines 120 - 150, Add a unit test to cover listener accept-handler priority propagation through createSocketAcceptHandler() and connListener::priority. The current SocketPrioritizationTest only verifies connSetPriority() and direct file-event priority updates, so extend the suite with a case that creates a listener, sets CONN_PRIORITY_HIGH before registering the accept handler, and asserts the resulting aeFileEvent in server.el has AE_PRIORITY_HIGH. Use the existing connection/listener setup patterns in test_socket_prioritization.cpp to keep the test focused and locate the new path in src/server.c.Source: Coding guidelines
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/ae.c`:
- Around line 532-535: Use aeProcessEvent()’s actual return value instead of
collapsing it to 1 in aeProcessEvents() and the priority-events loop. Update the
processed accumulator and both telemetry counters to add the number of callbacks
executed by aeProcessEvent(), so the return semantics and
*_events_processed_total stats reflect distinct read/write callback executions.
Locate the fix in aeProcessEvents() and the priority event handling around
aeProcessEvent().
- Around line 524-571: The event-loop dispatch in aeProcessEvents re-reads
eventLoop->events[fd].priority during both passes, which lets connSetPriority()
changes mid-cycle alter whether a fired entry runs again or gets skipped.
Snapshot each fired item’s priority once before callbacks start, and use that
captured value to decide whether the entry belongs to the high-priority pass or
the normal-priority pass for the rest of the cycle. Keep the fix localized
around the fired[j] handling in aeProcessEvents and preserve the existing
AE_NONE safety check.
In `@src/io_threads.c`:
- Line 473: The deadlock guard in drainIOThreadsQueue() still uses a combined
capacity check, which is unsafe now that responses are split between
io_shared_outbox[CONN_PRIORITY_HIGH] and io_shared_outbox[CONN_PRIORITY_NORMAL].
Update the logic to track pending completions separately by priority and compare
each shard against its own outbox queue_size so the high-priority path cannot
fill and block workers while the normal queue remains empty. Use the existing
priority-specific outbox symbols in io_threads.c to locate and adjust the check.
In `@src/replication.c`:
- Around line 1085-1088: Delay the CONN_PRIORITY_HIGH upgrade in replication.c
until after the SYNC/PSYNC request has passed all remaining validation and
rejection paths. The current connSetPriority call on c->conn runs too early and
can leave rejected requests permanently elevated, so move that logic below the
acceptance checks or explicitly restore normal priority on every early-return
rejection branch in the request handling flow.
In `@src/socket.c`:
- Around line 122-123: The connect path in src/socket.c currently ignores the
return value from aeCreateFileEvent(), so a failed writable-event registration
can still look like success and leave the connection stuck in
CONN_STATE_CONNECTING. Update the connect handling around aeCreateFileEvent() to
check for AE_ERR, and on failure unwind the fd/state the same way
connSocketSetWriteHandler() and connSocketSetReadHandler() do, so the socket is
not left half-initialized.
---
Nitpick comments:
In `@src/commands/client-kill.json`:
- Around line 229-235: Add a top-level history entry for the new QOS filter in
client-kill.json so it is documented alongside the other versioned additions,
and change the QOS argument definition from a generic string to a oneof. Model
the allowed values as pure-token sub-arguments for the two fixed options
(normal-priority and high-priority), matching the existing client-type/TYPE and
skipme/SKIPME patterns so COMMAND DOCS and COMMAND INFO expose the real choices.
Keep this as metadata only; no changes are needed in parseClientFiltersOrReply.
In `@src/commands/client-list.json`:
- Around line 211-217: The QOS argument metadata in client-list.json is missing
the same documentation and schema updates as client-kill.json: add a history
entry for the 10.0.0 introduction and change the definition from a generic
string to a oneof with the two fixed pure-token values. Update the QOS entry in
the client-list argument schema to match the pattern used by fixed-value filters
like client-type/TYPE and skipme/SKIPME, using the relevant metadata fields on
the QOS item.
In `@src/networking.c`:
- Around line 4912-4919: The client-filter parsing in networking.c should add a
negative counterpart for qos to match the existing not-* pattern used by type,
addr, laddr, user, name, flags, lib-name, lib-ver, db, capa, and ip. Update the
filter handling around the qos branch in the client filter parser (the same
logic used in the nearby helper that processes other filter tokens) so that
not-qos is recognized and mapped to the corresponding negated filter field,
mirroring how the other positive/negative pairs are implemented.
In `@src/tls.c`:
- Line 1201: The AE priority mapping is duplicated in both registerSSLEvent and
updateSSLEvent, so extract the conn->c.priority to
AE_PRIORITY_HIGH/AE_PRIORITY_NORMAL conversion into a small shared helper such
as connPriorityToAePriority and use it from both call sites. Keep the helper
near the existing SSL event logic in src/tls.c so the priority-derivation stays
consistent and any future mapping change only needs one update.
In `@src/unit/test_socket_prioritization.cpp`:
- Around line 120-150: Add a unit test to cover listener accept-handler priority
propagation through createSocketAcceptHandler() and connListener::priority. The
current SocketPrioritizationTest only verifies connSetPriority() and direct
file-event priority updates, so extend the suite with a case that creates a
listener, sets CONN_PRIORITY_HIGH before registering the accept handler, and
asserts the resulting aeFileEvent in server.el has AE_PRIORITY_HIGH. Use the
existing connection/listener setup patterns in test_socket_prioritization.cpp to
keep the test focused and locate the new path in src/server.c.
In `@tests/unit/socket-prioritization.tcl`:
- Around line 20-83: The socket prioritization test in the test body that uses
DEBUG SLEEP, after 50, and after 1100 is timing-sensitive and can be flaky on
slow runners; replace the fixed-delay synchronization with a more deterministic
readiness check or polling loop before sending the 250 SET commands and before
reading results. Keep the existing scope markers (needs:debug external:skip) and
adjust the test around the existing client setup, control_cl flow, and telemetry
assertions so it waits for the intended server state instead of assuming exact
sleep timing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: d4cc48f1-b170-409d-bb38-ff12aa49313d
📒 Files selected for processing (31)
src/ae.csrc/ae.hsrc/cluster.csrc/cluster_legacy.csrc/cluster_migrateslots.csrc/commands.defsrc/commands/client-kill.jsonsrc/commands/client-list.jsonsrc/config.csrc/connection.csrc/connection.hsrc/io_threads.csrc/module.csrc/networking.csrc/rdb.csrc/rdma.csrc/replication.csrc/sentinel.csrc/server.csrc/server.hsrc/socket.csrc/tls.csrc/unit/test_socket_prioritization.cppsrc/valkey-benchmark.ctests/sentinel/tests/19-socket-prioritization.tcltests/sentinel/tests/20-socket-prioritization-bench.tcltests/unit/cluster/socket-prioritization-bench.tcltests/unit/cluster/socket-prioritization.tcltests/unit/socket-prioritization-bench.tcltests/unit/socket-prioritization.tclvalkey.conf
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #4076 +/- ##
============================================
+ Coverage 80.12% 80.19% +0.07%
============================================
Files 189 190 +1
Lines 97253 97907 +654
============================================
+ Hits 77923 78521 +598
- Misses 19330 19386 +56
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/commands/client-list.json (1)
215-233: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding a
NOT-QOSnegative filter for consistency.Every other filter added in 9.0.0 (TYPE, ID, USER, ADDR, LADDR, NAME, IDLE, FLAGS, LIB-NAME, LIB-VER, DB, CAPA, IP) has a matching
NOT-*counterpart, butQOSdoesn't. If negative filtering is intended to be supported uniformly, this may be an oversight; if not, it's fine as is.🤖 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 `@src/commands/client-list.json` around lines 215 - 233, The QOS filter entry in client-list.json is missing a matching negative counterpart, unlike the other 9.0.0 filters. Update the client filter definition around the qos token to add a NOT-QOS variant if negative filtering is meant to be supported consistently, using the same pattern as the existing NOT-* filter entries and keeping the new symbol aligned with the QOS/NOT-QOS naming convention.tests/unit/cluster/manual-failover.tcl (1)
119-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUndocumented magic delay added twice.
Both instances add
after 200afterDEBUG SLEEP 2with no comment explaining why it's needed. Given this PR introduces event-loop priority scheduling for cluster/replication traffic, this is likely compensating for a timing change elsewhere, but that's not evident from the test alone.💬 Suggested comment addition
test "Make instance `#0` unreachable without killing it" { R 0 deferred 1 R 0 DEBUG SLEEP 2 + # Give the server a moment to actually start blocking on DEBUG SLEEP + # before considering it unreachable (needed due to QoS/event-loop + # scheduling changes). after 200 }Also applies to: 164-168
🤖 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 `@tests/unit/cluster/manual-failover.tcl` around lines 119 - 123, Add an inline comment near the repeated after 200 pause in the manual-failover test cases so the timing dependency is documented; the delay in the "Make instance `#0` unreachable without killing it" block (and the matching one in the other affected case) should explain that it intentionally gives the event loop time to process the scheduling/replication state change introduced by this PR. Update the relevant test helper/block in manual-failover.tcl so future readers understand why DEBUG SLEEP 2 is followed by the extra wait.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@src/commands/client-list.json`:
- Around line 215-233: The QOS filter entry in client-list.json is missing a
matching negative counterpart, unlike the other 9.0.0 filters. Update the client
filter definition around the qos token to add a NOT-QOS variant if negative
filtering is meant to be supported consistently, using the same pattern as the
existing NOT-* filter entries and keeping the new symbol aligned with the
QOS/NOT-QOS naming convention.
In `@tests/unit/cluster/manual-failover.tcl`:
- Around line 119-123: Add an inline comment near the repeated after 200 pause
in the manual-failover test cases so the timing dependency is documented; the
delay in the "Make instance `#0` unreachable without killing it" block (and the
matching one in the other affected case) should explain that it intentionally
gives the event loop time to process the scheduling/replication state change
introduced by this PR. Update the relevant test helper/block in
manual-failover.tcl so future readers understand why DEBUG SLEEP 2 is followed
by the extra wait.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 050e6393-b400-4521-adbe-50678dd0d060
📒 Files selected for processing (14)
src/ae.csrc/commands.defsrc/commands/client-kill.jsonsrc/commands/client-list.jsonsrc/connection.csrc/connection.hsrc/io_threads.csrc/rdma.csrc/replication.csrc/socket.csrc/tls.csrc/unit/test_socket_prioritization.cpptests/unit/cluster/manual-failover.tcltests/unit/socket-prioritization.tcl
🚧 Files skipped from review as they are similar to previous changes (9)
- src/tls.c
- src/rdma.c
- src/replication.c
- src/connection.h
- tests/unit/socket-prioritization.tcl
- src/socket.c
- src/commands.def
- src/ae.c
- src/io_threads.c
f892ee7 to
b8b71bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/rdma.c (1)
926-948: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnwind
server.elregistration ifserver.hp_elregistration fails (same pattern assrc/socket.c).In all three call sites, when the initial
aeCreateFileEventonserver.elsucceeds but the mirrored call onserver.hp_elfails (Lines 936-938, 1220-1222, 1246-1248), the function returnsC_ERRwhile leaving a live registration onserver.el. The caller believes the operation failed, butrdmaCMeventHandler/conn->type->ae_handlerwill still fire from the normal loop.🐛 Proposed fix (illustrated for `connRdmaSetRwHandler`; apply analogously to `connRdmaConnect`/`connRdmaBlockingConnect`)
if (conn->read_handler || conn->write_handler) { if (aeCreateFileEvent(server.el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) { return C_ERR; } if (connGetPriority(conn) == CONN_PRIORITY_HIGH && server.hp_el) { if (aeCreateFileEvent(server.hp_el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) { + aeDeleteFileEvent(server.el, conn->fd, AE_READABLE); return C_ERR; } } }Also applies to: 1197-1228, 1230-1258
🤖 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 `@src/rdma.c` around lines 926 - 948, The mirrored event registration in connRdmaSetRwHandler, connRdmaConnect, and connRdmaBlockingConnect can leave a live server.el file event behind if the server.hp_el aeCreateFileEvent call fails. Update each path to unwind the earlier server.el registration before returning C_ERR, matching the cleanup pattern used in src/socket.c, so conn->type->ae_handler is not left armed after a failed high-priority registration.src/replication.c (1)
1172-1175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the priority upgrade before the replica read handler is registered
connSetPriority(c->conn, CONN_PRIORITY_HIGH)only updates metadata;connSocketSetReadHandler()mirrors the readable event ontoserver.hp_elonly when the handler is installed while the connection is already high-priority. In this SYNC/PSYNC path, the replica read handler is not reinstalled afterward, so the connection stays on the normal loop.🤖 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 `@src/replication.c` around lines 1172 - 1175, Move the replica connection priority upgrade earlier in the SYNC/PSYNC flow so it happens before the replica read handler is installed. In the replication path around the existing connSetPriority(c->conn, CONN_PRIORITY_HIGH) logic, ensure the connection is marked high-priority before connSocketSetReadHandler() is called, or re-register the read handler after the priority change so it mirrors onto server.hp_el.src/cluster_migrateslots.c (1)
828-831: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet the priority before the read handler is registered
createClient()already installsreadQueryFromClient, so changingc->connto high priority here won’t move the existing readable event toserver.hp_el. Re-register the handler after the priority change, or move this upgrade earlier.🤖 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 `@src/cluster_migrateslots.c` around lines 828 - 831, The connection priority upgrade in createClient()/migrateslots handling is happening too late, after readQueryFromClient is already registered on the normal event loop. Move the connSetPriority(c->conn, CONN_PRIORITY_HIGH) step earlier in the flow, or explicitly re-register the read handler after the priority change so the existing readable event is moved to server.hp_el. Use the createClient and readQueryFromClient path to locate the right place to apply the priority change.
🧹 Nitpick comments (3)
src/unit/test_socket_prioritization.cpp (1)
298-345: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name/comment overstate what's verified.
The comment says this verifies
PROCESS_HP_EVENTS_IF_NEEDEDcheckshp_elon iterations where(itr_cnt & 3) == 0, but the assertions only checkg_execution_order[0] == 999— i.e., preemption at iteration 0. This doesn't actually validate the masked/periodic check behavior across later iterations (e.g., iteration 4, 8, etc.), so the test name is broader than its actual coverage.🤖 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 `@src/unit/test_socket_prioritization.cpp` around lines 298 - 345, The current test in SocketPrioritizationTest is broader in name/comment than what its assertions prove: it only confirms high-priority preemption at the first iteration via g_execution_order[0] == 999. Update the test so MultiIterationPreemptionCheckMask actually exercises and asserts the periodic PROCESS_HP_EVENTS_IF_NEEDED behavior in aeProcessEvents, by arranging enough normal events/iterations and checking that hp_el is serviced on later masked iterations as well (not just iteration 0). If you keep the test limited to first-iteration preemption, rename the test and comment to match that narrower behavior.tests/unit/cluster/socket-prioritization.tcl (2)
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify: use direct command dispatch instead of hand-built RESP.
Manually crafting the RESP array for
CLIENT SETNAMEis unnecessary and brittle — a single encoding mistake (length prefix,\r\n) silently breaks the test. The rest of this PR's tests (e.g.tests/unit/socket-prioritization.tclline 8:$c1 client setname qostestclient) invoke commands directly on the client object.♻️ Proposed simplification
- set normal_cl [valkey 127.0.0.1 [srv 0 port] 0 $::tls] - $normal_cl write "*3\r\n\$6\r\nCLIENT\r\n\$7\r\nSETNAME\r\n\$11\r\nclusternorm\r\n" - $normal_cl flush - assert_equal "OK" [$normal_cl read] + set normal_cl [valkey 127.0.0.1 [srv 0 port] 0 $::tls] + assert_equal "OK" [$normal_cl client setname clusternorm]🤖 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 `@tests/unit/cluster/socket-prioritization.tcl` around lines 11 - 18, The test in socket-prioritization.tcl is manually building RESP for CLIENT SETNAME instead of using the client API. Replace the hand-crafted write/flush/read sequence on normal_cl with a direct client command call, matching the style used elsewhere in the test suite (for example the client setname usage in socket-prioritization.tcl). Keep the existing assertions unchanged.
32-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPipeline load test has no correctness assertion.
Every write/read is wrapped in
catch { ... }andtotal_opsis never checked. The test will pass even if all writes fail or if prioritization causes data loss/incorrect cluster state — it only exercises the code path without verifying functional correctness. Compare to the analogous benchmark intests/unit/socket-prioritization.tcl(lines 112-118), which at least waits fordbsizeto reach an expected count.🤖 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 `@tests/unit/cluster/socket-prioritization.tcl` around lines 32 - 46, The pipeline load test in socket-prioritization.tcl only exercises writes through the load clients and swallows all failures via catch, so it never verifies correctness. Update the test around the load_clients loop and total_ops tracking to assert an expected outcome after the writes complete, using the existing socket-prioritization test helpers and dbsize-style verification pattern from the analogous test so failures in write/read or lost keys are caught.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/io_threads.c`:
- Around line 242-268: The teardown path in flushPendingIOResponses() can block
indefinitely when cleanupThreadResources() calls it with blocking enabled while
the main thread is already waiting in pthread_join(). Update the shutdown
behavior so IO-thread cleanup does not spin on mpscEnqueue for io_shared_outbox;
either make the cleanup flush non-blocking or ensure the main thread drains the
queue before cancel/join. Use the existing flushPendingIOResponses() and
cleanupThreadResources() flow to locate and adjust the shutdown path.
In `@src/socket.c`:
- Around line 274-286: `connSocketSetReadHandler` has the same unwind-on-failure
bug as `connSocketSetWriteHandler`: if `aeCreateFileEvent(server.hp_el, ...)`
fails after `server.el` registration succeeds, the first `AE_READABLE` event is
left installed. Update the failure path in this function to clean up the
already-created `server.el` event before returning `C_ERR`, mirroring the
rollback used for the high-priority registration and preserving consistent state
for `connGetPriority(conn) == CONN_PRIORITY_HIGH`.
- Around line 251-263: The writable-event setup in the connection registration
path leaves `server.el` armed when `aeCreateFileEvent(server.hp_el, ...)` fails
for a high-priority connection, so unwind the partial registration before
returning `C_ERR`. In the branch that handles `conn->write_handler` in
`src/socket.c`, add cleanup to remove the `AE_WRITABLE` event from `server.el`
and clear any write-handler state if the `server.hp_el` registration fails, so
`conn->type->ae_handler` is not left active on the main event loop after an
error.
In `@tests/unit/socket-prioritization.tcl`:
- Around line 91-118: The dbsize check in the socket prioritization test is too
strict because the pipelined SET traffic from the load clients introduces a new
unique key via the pipeline used in the replication/load section. Update the
assertion around wait_for_condition to account for the extra replicated key, or
change the load to reuse an existing key so the replica key count remains
unchanged. Use the existing test flow around the load_clients loop, replicaof
setup, and the dbsize checks on replica1/replica2 to keep the expectation
aligned with the actual replicated dataset.
- Line 69: The tags declaration in start_server is combining two intended tags
into one quoted string, so external:skip is not applied separately. Update the
tags list in the socket-prioritization benchmark to use separate list elements,
following the pattern used by the cluster/socket-prioritization test and the
single-tag form already present in this file. Ensure the start_server wrapper
still references socket-prioritization and external:skip as distinct tags.
---
Outside diff comments:
In `@src/cluster_migrateslots.c`:
- Around line 828-831: The connection priority upgrade in
createClient()/migrateslots handling is happening too late, after
readQueryFromClient is already registered on the normal event loop. Move the
connSetPriority(c->conn, CONN_PRIORITY_HIGH) step earlier in the flow, or
explicitly re-register the read handler after the priority change so the
existing readable event is moved to server.hp_el. Use the createClient and
readQueryFromClient path to locate the right place to apply the priority change.
In `@src/rdma.c`:
- Around line 926-948: The mirrored event registration in connRdmaSetRwHandler,
connRdmaConnect, and connRdmaBlockingConnect can leave a live server.el file
event behind if the server.hp_el aeCreateFileEvent call fails. Update each path
to unwind the earlier server.el registration before returning C_ERR, matching
the cleanup pattern used in src/socket.c, so conn->type->ae_handler is not left
armed after a failed high-priority registration.
In `@src/replication.c`:
- Around line 1172-1175: Move the replica connection priority upgrade earlier in
the SYNC/PSYNC flow so it happens before the replica read handler is installed.
In the replication path around the existing connSetPriority(c->conn,
CONN_PRIORITY_HIGH) logic, ensure the connection is marked high-priority before
connSocketSetReadHandler() is called, or re-register the read handler after the
priority change so it mirrors onto server.hp_el.
---
Nitpick comments:
In `@src/unit/test_socket_prioritization.cpp`:
- Around line 298-345: The current test in SocketPrioritizationTest is broader
in name/comment than what its assertions prove: it only confirms high-priority
preemption at the first iteration via g_execution_order[0] == 999. Update the
test so MultiIterationPreemptionCheckMask actually exercises and asserts the
periodic PROCESS_HP_EVENTS_IF_NEEDED behavior in aeProcessEvents, by arranging
enough normal events/iterations and checking that hp_el is serviced on later
masked iterations as well (not just iteration 0). If you keep the test limited
to first-iteration preemption, rename the test and comment to match that
narrower behavior.
In `@tests/unit/cluster/socket-prioritization.tcl`:
- Around line 11-18: The test in socket-prioritization.tcl is manually building
RESP for CLIENT SETNAME instead of using the client API. Replace the
hand-crafted write/flush/read sequence on normal_cl with a direct client command
call, matching the style used elsewhere in the test suite (for example the
client setname usage in socket-prioritization.tcl). Keep the existing assertions
unchanged.
- Around line 32-46: The pipeline load test in socket-prioritization.tcl only
exercises writes through the load clients and swallows all failures via catch,
so it never verifies correctness. Update the test around the load_clients loop
and total_ops tracking to assert an expected outcome after the writes complete,
using the existing socket-prioritization test helpers and dbsize-style
verification pattern from the analogous test so failures in write/read or lost
keys are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8184d4c2-f38b-4d8a-bfba-cb8a15d2b460
📒 Files selected for processing (20)
src/ae.csrc/ae.hsrc/cluster_legacy.csrc/cluster_migrateslots.csrc/commands.defsrc/commands/client-kill.jsonsrc/commands/client-list.jsonsrc/connection.csrc/connection.hsrc/io_threads.csrc/networking.csrc/rdma.csrc/replication.csrc/server.csrc/server.hsrc/socket.csrc/tls.csrc/unit/test_socket_prioritization.cpptests/unit/cluster/socket-prioritization.tcltests/unit/socket-prioritization.tcl
💤 Files with no reviewable changes (2)
- src/connection.c
- src/cluster_legacy.c
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/rdma.c (1)
926-948: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winUnwind
server.elregistration ifserver.hp_elregistration fails (same pattern assrc/socket.c).In all three call sites, when the initial
aeCreateFileEventonserver.elsucceeds but the mirrored call onserver.hp_elfails (Lines 936-938, 1220-1222, 1246-1248), the function returnsC_ERRwhile leaving a live registration onserver.el. The caller believes the operation failed, butrdmaCMeventHandler/conn->type->ae_handlerwill still fire from the normal loop.🐛 Proposed fix (illustrated for `connRdmaSetRwHandler`; apply analogously to `connRdmaConnect`/`connRdmaBlockingConnect`)
if (conn->read_handler || conn->write_handler) { if (aeCreateFileEvent(server.el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) { return C_ERR; } if (connGetPriority(conn) == CONN_PRIORITY_HIGH && server.hp_el) { if (aeCreateFileEvent(server.hp_el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) { + aeDeleteFileEvent(server.el, conn->fd, AE_READABLE); return C_ERR; } } }Also applies to: 1197-1228, 1230-1258
🤖 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 `@src/rdma.c` around lines 926 - 948, The mirrored event registration in connRdmaSetRwHandler, connRdmaConnect, and connRdmaBlockingConnect can leave a live server.el file event behind if the server.hp_el aeCreateFileEvent call fails. Update each path to unwind the earlier server.el registration before returning C_ERR, matching the cleanup pattern used in src/socket.c, so conn->type->ae_handler is not left armed after a failed high-priority registration.src/replication.c (1)
1172-1175: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove the priority upgrade before the replica read handler is registered
connSetPriority(c->conn, CONN_PRIORITY_HIGH)only updates metadata;connSocketSetReadHandler()mirrors the readable event ontoserver.hp_elonly when the handler is installed while the connection is already high-priority. In this SYNC/PSYNC path, the replica read handler is not reinstalled afterward, so the connection stays on the normal loop.🤖 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 `@src/replication.c` around lines 1172 - 1175, Move the replica connection priority upgrade earlier in the SYNC/PSYNC flow so it happens before the replica read handler is installed. In the replication path around the existing connSetPriority(c->conn, CONN_PRIORITY_HIGH) logic, ensure the connection is marked high-priority before connSocketSetReadHandler() is called, or re-register the read handler after the priority change so it mirrors onto server.hp_el.src/cluster_migrateslots.c (1)
828-831: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet the priority before the read handler is registered
createClient()already installsreadQueryFromClient, so changingc->connto high priority here won’t move the existing readable event toserver.hp_el. Re-register the handler after the priority change, or move this upgrade earlier.🤖 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 `@src/cluster_migrateslots.c` around lines 828 - 831, The connection priority upgrade in createClient()/migrateslots handling is happening too late, after readQueryFromClient is already registered on the normal event loop. Move the connSetPriority(c->conn, CONN_PRIORITY_HIGH) step earlier in the flow, or explicitly re-register the read handler after the priority change so the existing readable event is moved to server.hp_el. Use the createClient and readQueryFromClient path to locate the right place to apply the priority change.
🧹 Nitpick comments (3)
src/unit/test_socket_prioritization.cpp (1)
298-345: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest name/comment overstate what's verified.
The comment says this verifies
PROCESS_HP_EVENTS_IF_NEEDEDcheckshp_elon iterations where(itr_cnt & 3) == 0, but the assertions only checkg_execution_order[0] == 999— i.e., preemption at iteration 0. This doesn't actually validate the masked/periodic check behavior across later iterations (e.g., iteration 4, 8, etc.), so the test name is broader than its actual coverage.🤖 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 `@src/unit/test_socket_prioritization.cpp` around lines 298 - 345, The current test in SocketPrioritizationTest is broader in name/comment than what its assertions prove: it only confirms high-priority preemption at the first iteration via g_execution_order[0] == 999. Update the test so MultiIterationPreemptionCheckMask actually exercises and asserts the periodic PROCESS_HP_EVENTS_IF_NEEDED behavior in aeProcessEvents, by arranging enough normal events/iterations and checking that hp_el is serviced on later masked iterations as well (not just iteration 0). If you keep the test limited to first-iteration preemption, rename the test and comment to match that narrower behavior.tests/unit/cluster/socket-prioritization.tcl (2)
11-18: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify: use direct command dispatch instead of hand-built RESP.
Manually crafting the RESP array for
CLIENT SETNAMEis unnecessary and brittle — a single encoding mistake (length prefix,\r\n) silently breaks the test. The rest of this PR's tests (e.g.tests/unit/socket-prioritization.tclline 8:$c1 client setname qostestclient) invoke commands directly on the client object.♻️ Proposed simplification
- set normal_cl [valkey 127.0.0.1 [srv 0 port] 0 $::tls] - $normal_cl write "*3\r\n\$6\r\nCLIENT\r\n\$7\r\nSETNAME\r\n\$11\r\nclusternorm\r\n" - $normal_cl flush - assert_equal "OK" [$normal_cl read] + set normal_cl [valkey 127.0.0.1 [srv 0 port] 0 $::tls] + assert_equal "OK" [$normal_cl client setname clusternorm]🤖 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 `@tests/unit/cluster/socket-prioritization.tcl` around lines 11 - 18, The test in socket-prioritization.tcl is manually building RESP for CLIENT SETNAME instead of using the client API. Replace the hand-crafted write/flush/read sequence on normal_cl with a direct client command call, matching the style used elsewhere in the test suite (for example the client setname usage in socket-prioritization.tcl). Keep the existing assertions unchanged.
32-46: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPipeline load test has no correctness assertion.
Every write/read is wrapped in
catch { ... }andtotal_opsis never checked. The test will pass even if all writes fail or if prioritization causes data loss/incorrect cluster state — it only exercises the code path without verifying functional correctness. Compare to the analogous benchmark intests/unit/socket-prioritization.tcl(lines 112-118), which at least waits fordbsizeto reach an expected count.🤖 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 `@tests/unit/cluster/socket-prioritization.tcl` around lines 32 - 46, The pipeline load test in socket-prioritization.tcl only exercises writes through the load clients and swallows all failures via catch, so it never verifies correctness. Update the test around the load_clients loop and total_ops tracking to assert an expected outcome after the writes complete, using the existing socket-prioritization test helpers and dbsize-style verification pattern from the analogous test so failures in write/read or lost keys are caught.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/io_threads.c`:
- Around line 242-268: The teardown path in flushPendingIOResponses() can block
indefinitely when cleanupThreadResources() calls it with blocking enabled while
the main thread is already waiting in pthread_join(). Update the shutdown
behavior so IO-thread cleanup does not spin on mpscEnqueue for io_shared_outbox;
either make the cleanup flush non-blocking or ensure the main thread drains the
queue before cancel/join. Use the existing flushPendingIOResponses() and
cleanupThreadResources() flow to locate and adjust the shutdown path.
In `@src/socket.c`:
- Around line 274-286: `connSocketSetReadHandler` has the same unwind-on-failure
bug as `connSocketSetWriteHandler`: if `aeCreateFileEvent(server.hp_el, ...)`
fails after `server.el` registration succeeds, the first `AE_READABLE` event is
left installed. Update the failure path in this function to clean up the
already-created `server.el` event before returning `C_ERR`, mirroring the
rollback used for the high-priority registration and preserving consistent state
for `connGetPriority(conn) == CONN_PRIORITY_HIGH`.
- Around line 251-263: The writable-event setup in the connection registration
path leaves `server.el` armed when `aeCreateFileEvent(server.hp_el, ...)` fails
for a high-priority connection, so unwind the partial registration before
returning `C_ERR`. In the branch that handles `conn->write_handler` in
`src/socket.c`, add cleanup to remove the `AE_WRITABLE` event from `server.el`
and clear any write-handler state if the `server.hp_el` registration fails, so
`conn->type->ae_handler` is not left active on the main event loop after an
error.
In `@tests/unit/socket-prioritization.tcl`:
- Around line 91-118: The dbsize check in the socket prioritization test is too
strict because the pipelined SET traffic from the load clients introduces a new
unique key via the pipeline used in the replication/load section. Update the
assertion around wait_for_condition to account for the extra replicated key, or
change the load to reuse an existing key so the replica key count remains
unchanged. Use the existing test flow around the load_clients loop, replicaof
setup, and the dbsize checks on replica1/replica2 to keep the expectation
aligned with the actual replicated dataset.
- Line 69: The tags declaration in start_server is combining two intended tags
into one quoted string, so external:skip is not applied separately. Update the
tags list in the socket-prioritization benchmark to use separate list elements,
following the pattern used by the cluster/socket-prioritization test and the
single-tag form already present in this file. Ensure the start_server wrapper
still references socket-prioritization and external:skip as distinct tags.
---
Outside diff comments:
In `@src/cluster_migrateslots.c`:
- Around line 828-831: The connection priority upgrade in
createClient()/migrateslots handling is happening too late, after
readQueryFromClient is already registered on the normal event loop. Move the
connSetPriority(c->conn, CONN_PRIORITY_HIGH) step earlier in the flow, or
explicitly re-register the read handler after the priority change so the
existing readable event is moved to server.hp_el. Use the createClient and
readQueryFromClient path to locate the right place to apply the priority change.
In `@src/rdma.c`:
- Around line 926-948: The mirrored event registration in connRdmaSetRwHandler,
connRdmaConnect, and connRdmaBlockingConnect can leave a live server.el file
event behind if the server.hp_el aeCreateFileEvent call fails. Update each path
to unwind the earlier server.el registration before returning C_ERR, matching
the cleanup pattern used in src/socket.c, so conn->type->ae_handler is not left
armed after a failed high-priority registration.
In `@src/replication.c`:
- Around line 1172-1175: Move the replica connection priority upgrade earlier in
the SYNC/PSYNC flow so it happens before the replica read handler is installed.
In the replication path around the existing connSetPriority(c->conn,
CONN_PRIORITY_HIGH) logic, ensure the connection is marked high-priority before
connSocketSetReadHandler() is called, or re-register the read handler after the
priority change so it mirrors onto server.hp_el.
---
Nitpick comments:
In `@src/unit/test_socket_prioritization.cpp`:
- Around line 298-345: The current test in SocketPrioritizationTest is broader
in name/comment than what its assertions prove: it only confirms high-priority
preemption at the first iteration via g_execution_order[0] == 999. Update the
test so MultiIterationPreemptionCheckMask actually exercises and asserts the
periodic PROCESS_HP_EVENTS_IF_NEEDED behavior in aeProcessEvents, by arranging
enough normal events/iterations and checking that hp_el is serviced on later
masked iterations as well (not just iteration 0). If you keep the test limited
to first-iteration preemption, rename the test and comment to match that
narrower behavior.
In `@tests/unit/cluster/socket-prioritization.tcl`:
- Around line 11-18: The test in socket-prioritization.tcl is manually building
RESP for CLIENT SETNAME instead of using the client API. Replace the
hand-crafted write/flush/read sequence on normal_cl with a direct client command
call, matching the style used elsewhere in the test suite (for example the
client setname usage in socket-prioritization.tcl). Keep the existing assertions
unchanged.
- Around line 32-46: The pipeline load test in socket-prioritization.tcl only
exercises writes through the load clients and swallows all failures via catch,
so it never verifies correctness. Update the test around the load_clients loop
and total_ops tracking to assert an expected outcome after the writes complete,
using the existing socket-prioritization test helpers and dbsize-style
verification pattern from the analogous test so failures in write/read or lost
keys are caught.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8184d4c2-f38b-4d8a-bfba-cb8a15d2b460
📒 Files selected for processing (20)
src/ae.csrc/ae.hsrc/cluster_legacy.csrc/cluster_migrateslots.csrc/commands.defsrc/commands/client-kill.jsonsrc/commands/client-list.jsonsrc/connection.csrc/connection.hsrc/io_threads.csrc/networking.csrc/rdma.csrc/replication.csrc/server.csrc/server.hsrc/socket.csrc/tls.csrc/unit/test_socket_prioritization.cpptests/unit/cluster/socket-prioritization.tcltests/unit/socket-prioritization.tcl
💤 Files with no reviewable changes (2)
- src/connection.c
- src/cluster_legacy.c
🛑 Comments failed to post (5)
src/io_threads.c (1)
242-268: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash # Find call sites that invoke flushPendingIOResponses with blocking=1 rg -n 'flushPendingIOResponses\s*\(\s*1' src/io_threads.c rg -n 'flushPendingIOResponses' -C3 src/io_threads.cRepository: valkey-io/valkey
Length of output: 1362
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== src/io_threads.c around flush call sites ==" sed -n '250,290p' src/io_threads.c echo sed -n '350,390p' src/io_threads.c echo sed -n '760,790p' src/io_threads.c echo echo "== mpscEnqueue definition ==" rg -n 'mpscEnqueue|typedef.*mpsc' src include . -g '!*.o' -g '!*.a' -g '!*.so' -g '!*.dylib' -g '!*.dll' -C 3 echo echo "== search for io_shared_outbox / io_thread_ticket behavior ==" rg -n 'io_shared_outbox|io_thread_ticket|pending_io_responses' src include -C 2Repository: valkey-io/valkey
Length of output: 16080
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the surrounding implementation without executing repository code. echo "== src/io_threads.c around the blocking flush ==" cat -n src/io_threads.c | sed -n '260,285p' echo echo "== src/io_threads.c around the non-blocking flush paths ==" cat -n src/io_threads.c | sed -n '360,375p' echo cat -n src/io_threads.c | sed -n '760,775p' echo echo "== locate mpscEnqueue implementation ==" rg -n 'mpscEnqueue\s*\(' src include . -C 2 echo echo "== locate related queue/termination state ==" rg -n 'io_shared_outbox|io_thread_ticket|pending_io_responses|server\.crashed' src include -C 2Repository: valkey-io/valkey
Length of output: 10874
🏁 Script executed:
#!/bin/bash set -euo pipefail # Inspect the blocking flush path and the related call sites. echo "== src/io_threads.c around the blocking flush ==" cat -n src/io_threads.c | sed -n '260,285p' echo echo "== src/io_threads.c around the non-blocking flush paths ==" cat -n src/io_threads.c | sed -n '360,375p' echo cat -n src/io_threads.c | sed -n '760,775p' echo echo "== locate mpscEnqueue implementation ==" rg -n 'mpscEnqueue\s*\(' src include . -C 2 echo echo "== locate related queue/termination state ==" rg -n 'io_shared_outbox|io_thread_ticket|pending_io_responses|server\.crashed' src include -C 2Repository: valkey-io/valkey
Length of output: 10874
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== cleanup and thread shutdown wiring in src/io_threads.c ==" rg -n 'cleanupThreadResources|pthread_cleanup_push|pthread_cleanup_pop|pthread_join|server\.io_threads_num|stop' src/io_threads.c -C 3 echo echo "== mpscEnqueue implementation details ==" cat -n src/queues.c | sed -n '1,120p'Repository: valkey-io/valkey
Length of output: 9987
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== cleanup and thread shutdown wiring in src/io_threads.c ==" rg -n 'cleanupThreadResources|pthread_cleanup_push|pthread_cleanup_pop|pthread_join|server\.io_threads_num|stop' src/io_threads.c -C 3 echo echo "== mpscEnqueue implementation details ==" cat -n src/queues.c | sed -n '1,140p'Repository: valkey-io/valkey
Length of output: 10565
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== shutdownIOThread and resize path ==" cat -n src/io_threads.c | sed -n '395,475p' echo echo "== any explicit cancellation points in src/io_threads.c ==" rg -n 'pthread_testcancel|pthread_cancel|pthread_cleanup_push|pthread_cleanup_pop|sleep|usleep|sched_yield|nanosleep' src/io_threads.c -C 2Repository: valkey-io/valkey
Length of output: 4840
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== killIOThreads call sites ==" rg -n 'killIOThreads\s*\(' src -C 3 echo echo "== drainIOThreadsQueue definition/callers ==" rg -n 'drainIOThreadsQueue\s*\(' src -C 3Repository: valkey-io/valkey
Length of output: 3662
Avoid blocking flush during IO-thread teardown
flushPendingIOResponses(1)is called fromcleanupThreadResources()while the main thread is already inpthread_join(). Ifio_shared_outboxis full, the IO thread can spin without making progress because nothing is draining that queue during shutdown. Consider a non-blocking cleanup path or draining the queue before cancel/join.🤖 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 `@src/io_threads.c` around lines 242 - 268, The teardown path in flushPendingIOResponses() can block indefinitely when cleanupThreadResources() calls it with blocking enabled while the main thread is already waiting in pthread_join(). Update the shutdown behavior so IO-thread cleanup does not spin on mpscEnqueue for io_shared_outbox; either make the cleanup flush non-blocking or ensure the main thread drains the queue before cancel/join. Use the existing flushPendingIOResponses() and cleanupThreadResources() flow to locate and adjust the shutdown path.src/socket.c (2)
251-263: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unwind the
server.elregistration whenserver.hp_elregistration fails.If
aeCreateFileEventonserver.hp_elfails at Line 260-261, the function returnsC_ERR, but theAE_WRITABLEevent already created onserver.el(Line 257) is never removed, andconn->write_handlerremains set. The caller sees a failure, yet the write handler will still fire fromserver.el. This is a new failure mode introduced by this PR (previously there was only one loop to register on).🐛 Proposed fix
} else { if (aeCreateFileEvent(server.el, conn->fd, AE_WRITABLE, conn->type->ae_handler, conn) == AE_ERR) return C_ERR; if (connGetPriority(conn) == CONN_PRIORITY_HIGH) { - if (aeCreateFileEvent(server.hp_el, conn->fd, AE_WRITABLE, conn->type->ae_handler, conn) == AE_ERR) + if (aeCreateFileEvent(server.hp_el, conn->fd, AE_WRITABLE, conn->type->ae_handler, conn) == AE_ERR) { + aeDeleteFileEvent(server.el, conn->fd, AE_WRITABLE); return C_ERR; + } } }📝 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.if (!conn->write_handler) { aeDeleteFileEvent(server.el, conn->fd, AE_WRITABLE); if (connGetPriority(conn) == CONN_PRIORITY_HIGH) { aeDeleteFileEvent(server.hp_el, conn->fd, AE_WRITABLE); } } else { if (aeCreateFileEvent(server.el, conn->fd, AE_WRITABLE, conn->type->ae_handler, conn) == AE_ERR) return C_ERR; if (connGetPriority(conn) == CONN_PRIORITY_HIGH) { if (aeCreateFileEvent(server.hp_el, conn->fd, AE_WRITABLE, conn->type->ae_handler, conn) == AE_ERR) { aeDeleteFileEvent(server.el, conn->fd, AE_WRITABLE); return C_ERR; } } }🤖 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 `@src/socket.c` around lines 251 - 263, The writable-event setup in the connection registration path leaves `server.el` armed when `aeCreateFileEvent(server.hp_el, ...)` fails for a high-priority connection, so unwind the partial registration before returning `C_ERR`. In the branch that handles `conn->write_handler` in `src/socket.c`, add cleanup to remove the `AE_WRITABLE` event from `server.el` and clear any write-handler state if the `server.hp_el` registration fails, so `conn->type->ae_handler` is not left active on the main event loop after an error.
274-286: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Same unwind-on-failure gap as
connSocketSetWriteHandler.If registration on
server.hp_elfails at Line 283-284, the already-createdAE_READABLEevent onserver.el(Line 280-281) is left dangling while the function reportsC_ERR.🐛 Proposed fix
} else { if (aeCreateFileEvent(server.el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) return C_ERR; if (connGetPriority(conn) == CONN_PRIORITY_HIGH) { - if (aeCreateFileEvent(server.hp_el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) + if (aeCreateFileEvent(server.hp_el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) { + aeDeleteFileEvent(server.el, conn->fd, AE_READABLE); return C_ERR; + } } }📝 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.if (!conn->read_handler) { aeDeleteFileEvent(server.el, conn->fd, AE_READABLE); if (connGetPriority(conn) == CONN_PRIORITY_HIGH) { aeDeleteFileEvent(server.hp_el, conn->fd, AE_READABLE); } } else { if (aeCreateFileEvent(server.el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) return C_ERR; if (connGetPriority(conn) == CONN_PRIORITY_HIGH) { if (aeCreateFileEvent(server.hp_el, conn->fd, AE_READABLE, conn->type->ae_handler, conn) == AE_ERR) { aeDeleteFileEvent(server.el, conn->fd, AE_READABLE); return C_ERR; } } }🧰 Tools
🪛 Cppcheck (2.21.0)
[error] 284-284:
#error"Undefined or invalid BYTE_ORDER"(preprocessorErrorDirective)
🤖 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 `@src/socket.c` around lines 274 - 286, `connSocketSetReadHandler` has the same unwind-on-failure bug as `connSocketSetWriteHandler`: if `aeCreateFileEvent(server.hp_el, ...)` fails after `server.el` registration succeeds, the first `AE_READABLE` event is left installed. Update the failure path in this function to clean up the already-created `server.el` event before returning `C_ERR`, mirroring the rollback used for the high-priority registration and preserving consistent state for `connGetPriority(conn) == CONN_PRIORITY_HIGH`.tests/unit/socket-prioritization.tcl (2)
69-69: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Tags string merges two tags into one, losing
external:skip.
tags {"socket-prioritization external:skip"}is a single quoted string, so it parses as one list elementsocket-prioritization external:skip, not two separate tags. This differs from the correct multi-tag pattern used attests/unit/cluster/socket-prioritization.tclline 2 (tags {socket-prioritization external:skip cluster}, unquoted words) and from the single-tag pattern at line 5 of this same file (tags {"socket-prioritization"}). As written,external:skipis not recognized as its own tag, so this heavy nested-server benchmark isn't actually excluded when running against external servers.🐛 Proposed fix
-start_server {tags {"socket-prioritization external:skip"}} { +start_server {tags {"socket-prioritization" "external:skip"}} {📝 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.start_server {tags {"socket-prioritization" "external:skip"}} {🤖 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 `@tests/unit/socket-prioritization.tcl` at line 69, The tags declaration in start_server is combining two intended tags into one quoted string, so external:skip is not applied separately. Update the tags list in the socket-prioritization benchmark to use separate list elements, following the pattern used by the cluster/socket-prioritization test and the single-tag form already present in this file. Ensure the start_server wrapper still references socket-prioritization and external:skip as distinct tags.
91-118: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
dbsize assertion will never be satisfied — load writes an extra key beyond the populated 1000.
The primary is pre-populated with exactly 1000 keys (
benchkey:0..benchkey:999), but the load clients then repeatedlySET pipekey ...on the same primary connections (lines 91-110), adding a 1001st unique key that also replicates to both replicas. Thewait_for_conditionat lines 112-118 asserts[$replica1 dbsize] == 1000 && [$replica2 dbsize] == 1000, which will never hold once replication propagatespipekey, causing the test to time out and alwaysfail.🐛 Proposed fix
wait_for_condition 100 50 { [status $primary connected_slaves] == 2 && - [$replica1 dbsize] == 1000 && - [$replica2 dbsize] == 1000 + [$replica1 dbsize] == 1001 && + [$replica2 dbsize] == 1001 } else { fail "Replicas failed to complete sync during pipelined load" }📝 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.set val [string repeat "x" 256] set pipeline "" for {set p 0} {$p < 20} {incr p} { append pipeline "*3\r\n\$3\r\nSET\r\n\$7\r\npipekey\r\n\$256\r\n$val\r\n" } $replica1 replicaof $primary_host $primary_port $replica2 replicaof $primary_host $primary_port set total_ops 0 for {set iter 0} {$iter < 10} {incr iter} { foreach cl $load_clients { catch { $cl write $pipeline $cl flush incr total_ops 20 for {set r 0} {$r < 20} {incr r} { $cl read } } } } wait_for_condition 100 50 { [status $primary connected_slaves] == 2 && [$replica1 dbsize] == 1001 && [$replica2 dbsize] == 1001 } else { fail "Replicas failed to complete sync during pipelined load" }🤖 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 `@tests/unit/socket-prioritization.tcl` around lines 91 - 118, The dbsize check in the socket prioritization test is too strict because the pipelined SET traffic from the load clients introduces a new unique key via the pipeline used in the replication/load section. Update the assertion around wait_for_condition to account for the extra replicated key, or change the load to reuse an existing key so the replica key count remains unchanged. Use the existing test flow around the load_clients loop, replicaof setup, and the dbsize checks on replica1/replica2 to keep the expectation aligned with the actual replicated dataset.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/unit/test_socket_prioritization.cpp (2)
319-390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTiming-based assertion has a thin margin and may flake under CI load.
The test sleeps
usleep(1500)to exceed a presumed ~1000us poll-check threshold, only a 500us margin. Under a loaded CI runner, scheduler jitter could cause the sleep to not effectively separate poll checks, makingg_execution_orderassertions (Lines 366-375) flaky.Consider widening the margin (e.g., a few ms) to reduce flakiness risk.
🤖 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 `@src/unit/test_socket_prioritization.cpp` around lines 319 - 390, The timing assertions in MultiIterationPreemptionCheckMask are too close to the hp_el polling threshold and can flake under CI jitter. Update the test setup around aeProcessEvents, server.hp_el->last_poll_us, and the g_execution_order checks to use a wider timing margin or a more deterministic trigger so the high-priority event is clearly separated from the masked iterations. Keep the intent of verifying preemption, but avoid relying on a borderline usleep window.
52-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared static test globals reset only at test-end risk leaking state on assertion failure.
g_hp_pipe2_write_fd,g_normal_cb_count,g_execution_count/g_execution_orderare file-scope statics reused across tests, but reset happens near the end of each test body (e.g., Line 377). If an earlierASSERT_*in the same test fails and returns early, the reset is skipped and stale values (e.g., a danglingg_hp_pipe2_write_fdpointing to an fd closed in this test) leak into the next test that shares these globals, risking cascading/flaky failures unrelated to the actual bug under test.Move the reset of all shared statics into
SocketPrioritizationTest::SetUp()(already reset there per the summary for some counters) so each test starts from a guaranteed-clean state regardless of how the previous test exited.Also applies to: 359-377
🤖 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 `@src/unit/test_socket_prioritization.cpp` around lines 52 - 65, Shared file-scope test state in the socket prioritization tests is only being reset at the end of individual test bodies, so an early ASSERT failure can leak stale values into later tests. Move the reset for all shared statics, including g_hp_pipe2_write_fd, g_normal_cb_count, g_execution_count, and g_execution_order, into SocketPrioritizationTest::SetUp() so every test starts clean regardless of how the previous one exits. Keep the existing callback logic in testNormalEventCallbackMaskCheck unchanged apart from relying on the reset state.
🤖 Prompt for all review comments with 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.
Inline comments:
In `@src/connection.c`:
- Around line 172-195: connSetPriority() is registering high-priority file
descriptors on server.hp_el without removing or synchronizing the existing
server.el registration, which can let aeProcessEvents() dispatch the same fd
twice in one loop. Update connSetPriority() so switching to CONN_PRIORITY_HIGH
mirrors the fd registration state between server.el and server.hp_el, or
otherwise prevents an already-fired fd from being processed twice; use conn->fd,
server.el, server.hp_el, and the read/write handler checks to locate the change.
---
Nitpick comments:
In `@src/unit/test_socket_prioritization.cpp`:
- Around line 319-390: The timing assertions in
MultiIterationPreemptionCheckMask are too close to the hp_el polling threshold
and can flake under CI jitter. Update the test setup around aeProcessEvents,
server.hp_el->last_poll_us, and the g_execution_order checks to use a wider
timing margin or a more deterministic trigger so the high-priority event is
clearly separated from the masked iterations. Keep the intent of verifying
preemption, but avoid relying on a borderline usleep window.
- Around line 52-65: Shared file-scope test state in the socket prioritization
tests is only being reset at the end of individual test bodies, so an early
ASSERT failure can leak stale values into later tests. Move the reset for all
shared statics, including g_hp_pipe2_write_fd, g_normal_cb_count,
g_execution_count, and g_execution_order, into SocketPrioritizationTest::SetUp()
so every test starts clean regardless of how the previous one exits. Keep the
existing callback logic in testNormalEventCallbackMaskCheck unchanged apart from
relying on the reset state.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 4235a1d1-0c20-4558-a982-f694f1260a93
📒 Files selected for processing (6)
src/connection.csrc/connection.hsrc/server.csrc/server.hsrc/unit/test_socket_prioritization.cpptests/unit/cluster/socket-prioritization.tcl
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/unit/cluster/socket-prioritization.tcl
- src/server.h
- src/connection.h
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/unit/test_socket_prioritization.cpp (1)
52-65: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffTiming-dependent assertions may flake under CI load.
MultiIterationPreemptionCheckMaskrelies onusleep(1500)plus the exact(itr_cnt & 3) == 0cadence to force specific ordering ing_execution_order. Under scheduler jitter/CI contention this could produce nondeterministic ordering and intermittent failures.Consider decoupling the assertions from wall-clock timing (e.g., inject a mockable clock/hook for
last_poll_us) if flakiness surfaces in CI.Also applies to: 319-390
🤖 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 `@src/unit/test_socket_prioritization.cpp` around lines 52 - 65, The test logic in testNormalEventCallbackMaskCheck and the related MultiIterationPreemptionCheckMask flow depends on usleep-driven timing and the exact polling cadence, which can make g_execution_order assertions flaky under CI load. Refactor the test to avoid wall-clock scheduling assumptions by using a controllable or mockable clock/hook for last_poll_us, or another deterministic trigger, and update the assertions so they verify ordering through explicit events rather than timing luck.
🤖 Prompt for all review comments with 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.
Nitpick comments:
In `@src/unit/test_socket_prioritization.cpp`:
- Around line 52-65: The test logic in testNormalEventCallbackMaskCheck and the
related MultiIterationPreemptionCheckMask flow depends on usleep-driven timing
and the exact polling cadence, which can make g_execution_order assertions flaky
under CI load. Refactor the test to avoid wall-clock scheduling assumptions by
using a controllable or mockable clock/hook for last_poll_us, or another
deterministic trigger, and update the assertions so they verify ordering through
explicit events rather than timing luck.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: aabcf896-5bac-4158-bdc1-505fcc4fa25d
📒 Files selected for processing (6)
src/connection.csrc/connection.hsrc/server.csrc/server.hsrc/unit/test_socket_prioritization.cpptests/unit/cluster/socket-prioritization.tcl
🚧 Files skipped from review as they are similar to previous changes (4)
- src/server.c
- tests/unit/cluster/socket-prioritization.tcl
- src/server.h
- src/connection.h
018f2be to
58164d1
Compare
|
@coderabbitai summary |
✅ Action performedSummary regeneration triggered. |
1e11c9a to
91e6dbf
Compare
22a783b to
0927c6f
Compare
PR has been updated with following changes - 707e106
|
2da9e85 to
4bf2a2e
Compare
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
… feature Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
…os_' Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com>
2de1a78 to
089e2c0
Compare
|
This PR is very interesting. We previously implemented a similar approach internally, but did not continue with it because servicing high-priority system events consumed additional CPU and caused a noticeable increase in normal read/write latency and a significant QPS regression. This PR looks much more carefully designed, and the benchmark results are encouraging, so I think it is worth moving forward. I have a couple of questions about the benchmarks: Are the results in the top comment based on the latest implementation, which keeps a single main event loop with a separate priority polling backend? I understand an earlier version used two event loops. The steady-state results are particularly interesting: normal-command latency decreases while QPS increases. Have you investigated the reason for this improvement? For example, is it caused by smoother processing of replication and cluster traffic, reduced queue buildup, or changes in I/O-thread scheduling? I also noticed that the server was configured with io-threads 4. It would be useful to run the same benchmarks with I/O threads disabled. Many users run multiple Valkey server processes on the same machine rather than enabling I/O threads for each process. Testing this deployment model would help determine how much of the improvement comes from the event-loop prioritization itself versus the new high/normal-priority I/O-thread queues. |
Thank you for taking a look at this PR. and please find my comments inline.
Yes, please find the latest benchmarking on the latest code below.
Thats correct, it helped reduce the memory fragmentation and jmalloc arena churns, which lead to savings in main cpu cycles, which in-turn help process more qps.
Good point, I have attached benchmarking reuslts with and without iothreads offloading enabled for both standalone (a leader with 2 replicas) and cluster enabled (3 leaders with 2 replicas attached) below. |
Valkey QoS Steady State Benchmarking ResultsStandalone
Standalone IOthreads offloading disabledValkey node configBenchmark commandBaseline: QOS: Standalone IOthreads offloading enabledValkey node configBenchmark commandBaseline: QOS: Clustermode
Clustermode IOthreads offloading disabledValkey node configBenchmark commandBaseline: QOS: Clustermode IOthreads offloading enabledValkey node configBenchmark commandBaseline: QOS: |
madolson
added
major-decision-approved
Follow-on to #4076 which defines priority sources for anti-starvation. --------- Signed-off-by: Satheesha Gowda <satheesha.balaji@gmail.com> Co-authored-by: Jim Brunner <brunnerj@amazon.com>
Remove three entries whose changes shipped before 9.2.0, so 9.1 users don't see them advertised as new: - #3533 (dismissHashtable madvise size) was backported to 9.1 and is in the 9.1.0, 9.1.1 and 9.1.2 tags. It was added here by mistake. - #3551 (hashtable iterator invalidate on exhaustion) is likewise in 9.1.0; it just never made the 9.1 notes. - #3586 (valkey-cli --cluster fix slot spreading) is already documented in both the 9.0 and 9.1 release notes and shipped in 9.0.6 and 9.1.0. Fix the #4076 config name: the registered config is priority-preemptive-poll-interval-us, not preemptive-poll-interval-us, so the name as written would be rejected by CONFIG SET. Rewrite the #4005 entry, which described only the internal tagging. The user-facing surface is two new configs, priority-subnets and maxclients-reserved, which together cap normal clients at maxclients minus the reserved count so administrative clients keep guaranteed slots, plus a connected_priority_clients field in INFO clients. Move the sorted set B+ tree note (#4359) back to Performance. Listing it as a behavior change for its OBJECT ENCODING output while #3212 changes hash encoding output from hashtable to listpack in the Performance section was inconsistent; both now sit together. Drop the Redis comparison from the #4629 entry, which no other entry in the 9.0, 9.1 or 9.2 notes makes. Signed-off-by: Sarthak Aggarwal <sarthagg@amazon.com>
This PR focuses on implementing #3927 specifically on socket prioritization for system-critical events, such as cluster heartbeats, replication streams, and slot migrations.
1. Highlevel Summary & Scope
The Socket Prioritization feature introduces Quality of Service (QoS) scheduling into the Valkey engine. It establishes a dedicated, high-priority event handling path for system-critical events, such as cluster heartbeats, replication streams, and slot migrations to prevent Head-of-Line (HoL) blocking and starvation caused by heavy client traffic.
Subsystems in Scope (High Priority / CONN_PRIORITY_HIGH):
• Inbound replica connections (SYNC/PSYNC).
• Outbound primary replication links .
• Inbound accepted cluster links .
• Outbound cluster node interconnects .
• Inbound slot import jobs .
• Outbound slot export connections .
• Cached migration sockets.
2. User-Facing Changes
• New config
preemptive-poll-interval-us– defines the maximum elapsed time (in microseconds) between preemptive poll of the high-priority events while iterating through normal client events.•
CLIENT LIST / CLIENT INFO: Displays theHflag inflags, ex: when the replica link is high priority,flags=SH.•
CLIENT LIST flags H / CLIENT KILL flags H: Added support for filtering and selectively terminating connections by priority (flags H and negative filter not-flags H).•
Info Stats:Observability metrics•
Info debug:Observability metrics3. Platform Support & Compatibility Matrix
4. Core Architecture & Implementation Highlights
4.1. How Events Are Categorized by Priority
Connections are tagged as high priority either at creation time for outbound connections (such as cluster links, replication connections) or dynamically promoted for incoming connections (such as PSYNC/SYNC command or slot import) as high priority. all other incoming client connections are default tagged as normal priority.
4.2. Where Events Get Registered
• Dual-Tier Event Loops: Valkey maintains a Main Event Loop for normal client traffic and a nested High-Priority Event Loop for high priority events.
• Kernel Multiplexer Nesting: The operating system multiplexer file descriptor (e.g., epoll fd on Linux, kqueue fd on macOS/BSD) of the high-priority event loop is registered directly inside the main event loop.
• Virtual Routing: When a socket registers for read or write readiness, its priority tag automatically directs the registration to the corresponding event loop.
4.3. How Events Get Processed
io_shared_inbox[CONN_PRIORITY_NORMAL], io_shared_inbox[CONN_PRIORITY_HIGH]andio_shared_outbox[CONN_PRIORITY_COUNT], io_shared_outbox[CONN_PRIORITY_HIGH], such that high-priority queues are drained first, then preemptively polls high priority events while processing normal queues to be consistent with the behavior when io-threads disabled.5. Performance Benchmarking results.
There are two types of tests conducted to understand the performance characteristics of this feature.
1 Smoke Test This test has conducted with 94 concurrent active connections , to keep the latency under sub millisecond to mimic the happy path workload.
2 Stress Test This teas has conducted with 940 concurrent active connections to saturate CPU to validate the benefits of QoS when system under heavy load.
TLDR;
As the number of concurrent clients increases, under 100% cpu load, QoS yields better results , i.e significantly reduces the atomic slot migration duration and cut down number of full syncs, without introducing any noticeable regression.
I believe thats the core of the problem we are trying to address with QoS, when system is under heavy load prioritize cluster stability/availability over normal client traffic.
5.1 Smoke test (Under minimum load to keep the latency sub-milliseconds)
Compare latency, throughput, and QoS benefits against base line with ~100 concurrent clients under following 3 circumstances...
5.1.1 Setup
5.1.2 Valkey node config
5.1.3 Valkey cluster nodes
5.1.4 Cluster Steady state
Baseline
QoS
5.1.5 Cluster Full Sync Loop
Baseline
QoS
5.1.6 Cluster Atomic Slot Migration
Baseline
QoS
5.1.7 Executive Summary
5.2 Stress test (CPU saturated)
Compare latency, throughput, and QoS benefits against base line with ~1000 concurrent clients under following 3 circumstances...
5.2.1 Setup
5.2.2 Valkey node config
5.2.3 Valkey cluster nodes
5.2.4 Cluster Steady state
Baseline
QoS
5.2.5 Cluster Full Sync Loop
Baseline
QoS
5.2.6 Cluster Atomic Slot Migration
Baseline
QoS
5.2.7 Executive Summary