Steady state replication throttling POC#3
Conversation
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
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>
430a560 to
72ebc75
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
72ebc75 to
5904359
Compare
Signed-off-by: harrylin98 <harrylin980107@gmail.com>
JimB123
left a comment
There was a problem hiding this comment.
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?
| createBoolConfig("repl-mptcp", NULL, IMMUTABLE_CONFIG, server.repl_mptcp, 0, isValidMptcp, NULL), | ||
| createBoolConfig("repl-diskless-sync", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, server.repl_diskless_sync, 1, NULL, NULL), | ||
| createBoolConfig("dual-channel-replication-enabled", NULL, DEBUG_CONFIG | MODIFIABLE_CONFIG, server.dual_channel_replication, 0, NULL, NULL), | ||
| createBoolConfig("repl-throttle", NULL, MODIFIABLE_CONFIG, server.repl_throttle, 0, NULL, NULL), |
There was a problem hiding this comment.
Need a few people to think about naming. Specifically, this throttler is for steady-state replication only.
Maybe this is ok. At Amazon, we have fine grained control, with different throttlers configured for different phases of full sync and steady-state.
| /* Throttling */ | ||
| long long total_throttled_commands; /* Total commands deferred by the throttle framework */ | ||
| int repl_throttle; /* Enable replication throttle */ |
There was a problem hiding this comment.
The throttling unit can own these. No need to add to server.
Also, repl_throttle - check naming. This is a config value. Probably want a name like repl_throttle_enabled.
I'd recommend:
total_throttled_commandsis a metric related to the framework. I'd add this to throttle.h
struct throttle_metrics {
long long total_throttled_commands;
};
extern struct throttle_metrics throttle_metrics; // the actual struct is defined in the .c file
repl_throttleis a config and is specific to replication throttling. I'd add this to throttle_repl.h
struct throttle_repl_config {
int throttle_repl_enabled;
};
extern struct throttle_repl_config throttle_repl_config;
| @@ -0,0 +1,46 @@ | |||
| #ifndef THROTTLE_H | |||
| long oldest_client_delay_us; | ||
| } throttleMetrics; | ||
|
|
||
| /* Public API */ |
There was a problem hiding this comment.
The public API should be fully documented.
|
|
||
| void throttle_deregister(int id); | ||
|
|
||
| void *throttle_setPrivData(int id, void *new_priv_data); |
There was a problem hiding this comment.
This was added later at Amazon. Do we really need this?
|
|
||
| void tokenBucket_capDebt(tokenBucket *bucket, double max_debt); | ||
| double tokenBucket_add(tokenBucket *bucket, double tokens); | ||
| double tokenBucket_replenish(tokenBucket *bucket); |
There was a problem hiding this comment.
No need for this to be an API. This can happen intrinsically.
| bool tokenBucket_canConsume(tokenBucket *bucket, double tokens); | ||
| void tokenBucket_consume(tokenBucket *bucket, double tokens); |
There was a problem hiding this comment.
Why do we need separate canConsume and consume functions?
Eliminate canConsume and change consume to return a bool.
| bool tokenBucket_canConsume(tokenBucket *bucket, double tokens); | ||
| void tokenBucket_consume(tokenBucket *bucket, double tokens); | ||
| double tokenBucket_msUntilAvailable(tokenBucket *bucket, double tokens); | ||
| void tokenBucket_halt(tokenBucket *bucket); |
There was a problem hiding this comment.
Why do we need a "halt" function. Looks like overdesign.
| double tokenBucket_getBucketSize(tokenBucket *bucket); | ||
| double tokenBucket_getMaxBurstTime(tokenBucket *bucket); |
| typedef struct tokenBucket tokenBucket; | ||
|
|
||
| // APIs | ||
| tokenBucket *tokenBucket_create(double tokens_per_sec, double max_burst_time_secs, bucketSizeFunc *bucket_size_func); |
There was a problem hiding this comment.
What is bucket_size_function? How is it used? Why is it needed? Is this overdesign?
No description provided.