Steady state replication throttling POC by harrylin98 · Pull Request #3 · harrylin98/valkey_forked

harrylin98 · 2026-06-23T23:16:51Z

No description provided.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

…ths (valkey-io#3600) The `pending_command` flag indicates that a client has a fully parsed command ready for execution. This update ensures that the flag is set/cleared consistently across different execution paths. --------- Signed-off-by: harrylin98 <harrylin980107@gmail.com>

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

JimB123

I focused on the APIs. Big comments:

APIs (.h files) need solid documentation
Avoid pulling in Amazon overdesign
Consider each thing in the .h file. Ask does this need to be part of the API? Why?

JimB123 · 2026-06-24T22:30:30Z

+    long oldest_client_delay_us;
+} throttleMetrics;
+
+/* Public API */


The public API should be fully documented.

+1, This is applicable to all the .h files we have included here.

JimB123 · 2026-06-24T22:42:50Z

+
+void tokenBucket_capDebt(tokenBucket *bucket, double max_debt);
+double tokenBucket_add(tokenBucket *bucket, double tokens);
+double tokenBucket_replenish(tokenBucket *bucket);


No need for this to be an API. This can happen intrinsically.

dubey02 · 2026-07-06T18:30:01Z

+    long oldest_client_delay_us;
+} throttleMetrics;
+
+/* Public API */


+1, This is applicable to all the .h files we have included here.

dubey02 · 2026-07-06T19:14:40Z

+    tokenBucket *bucket;
+    list *client_queue;
+    listNode *ln; /* my node in throttlerList */
+    monotime rate_below_guardrail_since;


Isn't this field more related to repl-throttle specifically? I think a generic throttle framework shouldn't care about guardrails. Can we please track it in throttle-repl.c somewhere ?

dubey02 · 2026-07-06T19:20:32Z

+static list *throttlerList = NULL;
+static hashtable *metricsTable = NULL;
+
+typedef struct throttler {


One liner Documentation on structures and its members would help the readability.

dubey02 · 2026-07-06T21:52:00Z

+    setRate(t, ops_per_sec);
+}
+
+double throttle_adjustRate(int id, double multiplier) {


Is this a generic way of adjusting rate? Or Is this more related to a replication specific adjustment and not a generic one? what is multiplier here?

adjustRate is in the framework layer. The caller only decides "increase or decrease" and passes a multiplier. The framework handles the edge cases (halted recovery, minimum step, TPS floor).

I think different throttler would like to have their own adjust rate function. I don't think this is generic enough to put in framework layer.

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

dubey02 · 2026-07-31T18:58:31Z

+#define MAX_WAIT_TIME_MS 100                 /* max ms before rescheduling timer */
+#define MAX_UNTHROTTLE_PROCESSING_TIME_MS 10 /* max ms spent unthrottling per timer fire */
+#define THROTTLE_CLEANUP_ID (-1)             /* sentinel: throttler deregistered, draining queue */
+#define THROTTLE_OPS_PER_MIN_GUARDRAIL 6     /* 0.1 TPS - report when rate stays below this */
+#define TPS_WINDOW_SEC 5                     /* rolling window for incoming TPS measurement */
+#define EPSILON 0.0001                       /* values below this are treated as zero */
+#define TOKENS_BURST_RATE_SEC 0.1            /* burst capacity in seconds of sustained rate */
+#define MIN_ADJUST_AFTER_DISABLE 100.0       /* initial rate when recovering from halted state */


Do we plan to make these configurable in future ?

dubey02 · 2026-07-31T19:50:42Z

+    if (listLength(t->client_queue) == 0) {
+        serverAssert(t->time_event_id != AE_DELETED_EVENT_ID);
+        aeDeleteTimeEvent(server.el, t->time_event_id);
+        t->time_event_id = AE_DELETED_EVENT_ID;
+        if (t->id == THROTTLE_CLEANUP_ID) freeThrottler(t);
+    }


I think there is a double free here when a deregistered throttler drains its last client from inside the timer. throttle_removeClient frees the throttler when the queue empties and the id is cleanup, but the timer calls throttle_removeClient in its drain loop and keeps using t afterwards. So on the last client, t gets freed inside removeClient, then the loop condition reads t->client_queue from freed memory, and the block after the loop calls freeThrottler(t) a second time. You can hit it by deregistering a throttler that still has queued clients and letting the timer drain it, should show up right away under ASAN. I would suggest making the timer the only place that frees the throttler, have removeClient just dequeue, and break out of the drain loop as soon as the queue is empty. The one case that needs extra care then is the last client disconnecting while the timer is not running, maybe a 0ms one-shot timer to reap it.

dubey02 · 2026-07-31T20:02:32Z

+
+        if (t->criteria_proc(c, t->priv_data)) {
+            match_count++;
+            tpsCalculator_record(t->metrics->incoming_tps, 1);


Since throttlers sharing a metrics_name point at the same metricsEntry, this records once per matching throttler on the same calculator. So if two throttlers in a group match the same command, incoming TPS gets counted twice for one command. That inflated value then feeds the floor in adjustRate, meaning the rate can never be clamped below roughly 2x the real traffic in exactly the multi-instance case the shared metrics feature is meant for. I think this should record once per command per metrics group, not per throttler.

dubey02 · 2026-07-31T20:23:53Z

+        if (isThrottlerActive()) uninstallThrottler();
+        return;
+    }
+    if (!throttle_repl_config.steady_state_repl_throttle_enabled && !isThrottlerActive()) return;


If an operator runs CONFIG SET steady-state-repl-throttle-enabled no while the throttler is active, this falls through, re-evaluates COB, and can keep throttling harder, no ? We might need to have something like
if (!enabled) { if (isThrottlerActive()) uninstallThrottler(); return; }. Could you please check this scenario.

dubey02 · 2026-07-31T20:30:39Z

+/* Criteria: throttle commands that generate replication traffic. */
+static bool criteriaProc(client *c, void *priv_data) {
+    UNUSED(priv_data);
+    if (c->cmd->flags & (CMD_WRITE | CMD_MAY_REPLICATE)) return true;


I think this still needs a guard for the primary link. The criteria matches any WRITE or MAY_REPLICATE command but on a replica the replication stream from the primary arrives as exactly those commands on the primary-link client. The throttler is only uninstalled on demotion from serverCron, which runs this every 100ms, so there's a window where a just-demoted node still has an active throttler while it's already receiving the new primary's stream. Those commands match the criteria, throttleClientIfNeeded doesn't exclude the link (the !c->conn check only skips fake clients), and the primary link ends up queued with its read handler removed. This can stall the replication stream behind a user traffic throttle during a failover window. I would suggest adding if (c->flag.primary || c->flag.replica) return false; here or even better exclude these at the framework level in throttleClientIfNeeded so no future criteria proc can repeat this mistake.

JimB123 · 2026-07-31T22:43:35Z

+ *    1.  Adjust throttling at a regular interval > 250ms.  Adjusting the throttle too fast will
+ *        result in large throttling swings before an observed metric has a chance to change.
+ *        This can easily create a hysteresis problem.  The current incoming rate is based on a
+ *        5-second window and will not update faster than 250ms.


Don't we use 100ms for steady-state throttling? Suspicious.

JimB123 · 2026-07-31T22:45:19Z

+ *    2.  Set a target for the observed metric.  As the observed metric approaches the target, make
+ *        progressively smaller changes to the rate. */


Need to be careful with this guidance. Larger numbers (when further from the target) would result in hysteresis. I've had good luck with a constant value (5%).

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

Co-authored-by: Jim Brunner <brunnerj@amazon.com> Signed-off-by: Harry Lin <49881386+harrylin98@users.noreply.github.com>

harrylin98 added 2 commits June 23, 2026 15:54

Steady-state replication throttling and throttling infrastructure

7bfc426

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

Merge branch 'unstable' into rate_limit_poc

3b5cfe2

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

github-actions Bot assigned harrylin98 Jun 23, 2026

harrylin98 and others added 2 commits June 23, 2026 16:25

zombie connection detection from forkless branch

1a60842

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

harrylin98 force-pushed the rate_limit_poc branch 4 times, most recently from 430a560 to 72ebc75 Compare June 24, 2026 04:54

Format and correct comments

5904359

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

harrylin98 force-pushed the rate_limit_poc branch from 72ebc75 to 5904359 Compare June 24, 2026 05:29

Merge branch 'unstable' into rate_limit_poc

51219f7

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

JimB123 suggested changes Jun 24, 2026

View reviewed changes

dubey02 reviewed Jul 6, 2026

View reviewed changes

Address comments

ec331ac

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

harrylin98 force-pushed the rate_limit_poc branch from 041c81d to ec331ac Compare July 10, 2026 19:11

Merge branch 'unstable' into rate_limit_poc

295ab2e

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

harrylin98 force-pushed the rate_limit_poc branch 8 times, most recently from 802f610 to 9a1ca67 Compare July 14, 2026 08:44

Add documentation

bfe7d9e

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

harrylin98 force-pushed the rate_limit_poc branch 2 times, most recently from e99ce3d to 53b098f Compare July 21, 2026 00:16

harrylin98 force-pushed the rate_limit_poc branch 2 times, most recently from 81ade19 to c2a2485 Compare July 21, 2026 23:30

harrylin98 force-pushed the rate_limit_poc branch 8 times, most recently from 36f9e96 to 491ac66 Compare July 27, 2026 22:43

Adding unit tests

c1266dc

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

harrylin98 force-pushed the rate_limit_poc branch from 491ac66 to c1266dc Compare July 28, 2026 06:46

harrylin98 requested review from JimB123 and dubey02 July 28, 2026 20:20

harrylin98 force-pushed the rate_limit_poc branch from 67c7b2e to e12eb1b Compare July 29, 2026 00:17

dubey02 reviewed Jul 31, 2026

View reviewed changes

JimB123 reviewed Jul 31, 2026

View reviewed changes

harrylin98 force-pushed the rate_limit_poc branch 3 times, most recently from 1f5d591 to 07189f6 Compare August 3, 2026 16:39

Merge branch 'unstable' into rate_limit_poc

25c7d9d

Signed-off-by: harrylin98 <harrylin980107@gmail.com>

harrylin98 force-pushed the rate_limit_poc branch from 07189f6 to 25c7d9d Compare August 3, 2026 17:06

Apply suggestions from code review

762ed68

Co-authored-by: Jim Brunner <brunnerj@amazon.com> Signed-off-by: Harry Lin <49881386+harrylin98@users.noreply.github.com>

		* 2. Set a target for the observed metric. As the observed metric approaches the target, make
		* progressively smaller changes to the rate. */

Conversation

harrylin98 commented Jun 23, 2026

Uh oh!

JimB123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants