-
Notifications
You must be signed in to change notification settings - Fork 702
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[feature] sentinel use info sentinel command to run faster #1511
base: unstable
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #1511 +/- ##
============================================
- Coverage 70.83% 70.83% -0.01%
============================================
Files 120 120
Lines 64911 64930 +19
============================================
+ Hits 45982 45993 +11
- Misses 18929 18937 +8
|
Signed-off-by: wei.kukey <[email protected]>
2837997
to
2a5d3f6
Compare
@@ -3373,6 +3373,9 @@ standardConfig static_configs[] = { | |||
createSpecialConfig("replicaof", "slaveof", IMMUTABLE_CONFIG | MULTI_ARG_CONFIG, setConfigReplicaOfOption, getConfigReplicaOfOption, rewriteConfigReplicaOfOption, NULL), | |||
createSpecialConfig("latency-tracking-info-percentiles", NULL, MODIFIABLE_CONFIG | MULTI_ARG_CONFIG, setConfigLatencyTrackingInfoPercentilesOutputOption, getConfigLatencyTrackingInfoPercentilesOutputOption, rewriteConfigLatencyTrackingInfoPercentilesOutputOption, NULL), | |||
|
|||
/* Capabalities */ | |||
createBoolConfig("info-simple-for-sentinel", NULL, IMMUTABLE_CONFIG, server.info_simple_for_sentinel, 1, NULL, NULL), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this config only for sentinel? If yes, I think it should be moved to sentinel.c, we have some specific config parameters for sentinel node.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes,this config only for sentinel, but it's mean valkey has a certain ability that sentinel could send diff command to collect instance stats.
It's a server capability. I think it can't move to sentinel.c.
I am wondering why you consider the performance of the INFO command relevant in this case (or to be more precise what you mean by "large cluster"): Sentinels do not scale by sharding, all members of the sentinel cluster manage all primaries. The reason to have multiple sentinel instances is to increase robustness. By default, each sentinel instance sends an INFO command every 10 seconds to each monitored Valkey node. (And IIRC, one can change that timing only by using a debug command) So, even if we assume that we have 9 Sentinels (I assume most deployment use 3 or 5), this means that each Valkey node receives around one INFO command per second. For example:
Which are 54 INFO commands per minute. This means that the performance of the INFO command is completely negligible. @kukey Could you explain in which scenario the performance improvement achieved by this PR becomes relevant? |
@gmbnomis We use 3-6 sentinel to manage thousands of valley isntance, if sentinel use this simple info, could save more cpu and bandwidth; and to the client and server, it`s more stable and smoother latency |
I see. So, the performance improvements of the INFO command given in the PR description are not really the point of this PR. It is about sentinel performance. I noticed that the I am wondering if the improvements you see may be caused (partly) by the omission of this field? |
In sentinel cluster, sentinels will send info command to collect valkey-server state;
but in large cluster, the info command response is heavy and slow, so we need some
simple info response for sentinel.
changes in this pr:
valkey-server:
sentinel:
performence test:
valkey-benchmark -q -n 1000000 info
info: 65841.45 requests per second, p50=0.703 msec
valkey-benchmark -q -n 1000000 info sentinel
info sentinel: 236910.67 requests per second, p50=0.111 msec