-
Notifications
You must be signed in to change notification settings - Fork 694
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
Cache CLUSTER SLOTS response for improving throughput and reduced latency. #53
Conversation
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
18c139e
to
1836d9f
Compare
Added Sign-off to the PR |
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.
This improves the cluster slots response significantly in a scenario of defragmented slots (extreme case is 1-1, 3-3 and so on on shard 1 and remaining on shard 2) by caching the output and share it across client connection(s). Lot's of clients are still using CLUSTER SLOTS
command for topology discovery. So, it would be pretty beneficial for them.
Regarding the PR, one of the key challenges is discovering any state change of the cluster topology and invoking clearCachedClusterSlotsResp
to recompute the CLUSTER SLOTS
response. Would be nice to hear if there are any better way to approach this.
@valkey-io/core-team Please take a look.
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.
I have not thought through all the cache invalidation paths. Need to take a closer look next.
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
@roshkhatri There is a memory sanitization error. Looks like we're leaking some memory now. |
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## unstable #53 +/- ##
============================================
+ Coverage 69.67% 69.83% +0.15%
============================================
Files 109 109
Lines 61801 61864 +63
============================================
+ Hits 43062 43200 +138
+ Misses 18739 18664 -75
|
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.
I think the one open question left is @PingXie concern about using the original client vs a new caching client, so would like to close that before merging.
@roshkhatri had tried the original client approach first. The change becomes a bit complex due to maintaining the start index of the |
I would like to see more information about this. I'm still not really convinced this is true. |
we add a dummy node using |
I don't think this is where the complexity is stemming from. My original concern was that there are a lot of other edge cases around client that could possibly corrupt the output such as disabling the client reply or hitting the CoB limits. Having a dedicated client side-steps a lot of this complexity since we control that secondary client. |
Yes, that's also one of the point we had discussed internally 👍 . I'm aligned with current approach if there is no strong concern would like to merge this in. |
@roshkhatri Should be able to share the diff. |
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
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.
So my sanity check for when we need to invalidate:
- When announced IP or hostnames config changed ✅
- When preferred endpoint type changes ✅
- TLS cluster can not change, so the default can not change.
- When a node updates it's slow ownership ✅
- When a failover happens ✅
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
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.
OK, I'm happy with all the updates. @PingXie Let me know if you want to take another look at it before merging.
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.
mostly readability comments.
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
…es in cluster_legacy file Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
Signed-off-by: Roshan Khatri <rvkhatri@amazon.com>
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.
Thanks for working on it. 👍
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.
There is one last nitpick - not a blocker to me.
I like this PR :-). Thanks @roshkhatri!
We're improving a command that's deprecated. :) |
Maybe we should reconsider the decision :). |
@zuiderkwast @PingXie I think we should un-deprecate it, @hpatro and I were just talking about it. He'll open PR and we can discuss there. |
Have you bugged our office ? :D |
…ency. (valkey-io#53) This commit adds a logic to cache `CLUSTER SLOTS` response for reduced latency and also updates the cache when a change in the cluster is detected. Historically, `CLUSTER SLOTS` command was deprecated, however all the server clients have been using `CLUSTER SLOTS` and have not migrated to `CLUSTER SHARDS`. In future this logic can be added to any other commands to improve the performance of the engine. --------- Signed-off-by: Roshan Khatri <rvkhatri@amazon.com> convert centos 7 tests to almalinux 8
Undeprecate cluster slots command. This command is widely used by clients to form the cluster topology and with the recent change to improve performance of `CLUSTER SLOTS` command via #53 as well as us looking to further improve the usability via #517, it makes sense to undeprecate this command. --------- Signed-off-by: Harkrishn Patro <harkrisp@amazon.com>
In valkey-io#53, we will cache the CLUSTER SLOTS response to improve the throughput and reduct the latency. In the code snippet below, the second cluster slots will use the old hostname: ``` config set cluster-preferred-endpoint-type hostname config set cluster-announce-hostname old-hostname.com multi cluster slots config set cluster-announce-hostname new-hostname.com cluster slots exec ``` When updating the hostname, in updateAnnouncedHostname, we will set CLUSTER_TODO_SAVE_CONFIG and we will do a clearCachedClusterSlotsResponse in clusterSaveConfigOrDie, so harmless in most cases. We will call clearCachedClusterSlotsResponse in updateClusterAnnouncedPort, so it is reasonable to also call clearCachedClusterSlotsResponse in updateClusterHostname. Signed-off-by: Binbin <binloveplay1314@qq.com>
#564) In #53, we will cache the CLUSTER SLOTS response to improve the throughput and reduct the latency. In the code snippet below, the second cluster slots will use the old hostname: ``` config set cluster-preferred-endpoint-type hostname config set cluster-announce-hostname old-hostname.com multi cluster slots config set cluster-announce-hostname new-hostname.com cluster slots exec ``` When updating the hostname, in updateAnnouncedHostname, we will set CLUSTER_TODO_SAVE_CONFIG and we will do a clearCachedClusterSlotsResponse in clusterSaveConfigOrDie, so harmless in most cases. Move the clearCachedClusterSlotsResponse call to clusterDoBeforeSleep instead of scheduling it to be called in clusterSaveConfigOrDie. Signed-off-by: Binbin <binloveplay1314@qq.com>
PR #53 introduced the cache of CLUSTER SLOTS response, but the cache has some problems for different types of clients: 1. the RESP version is wrongly ignored: ``` $./valkey-cli 127.0.0.1:6379> cluster slots 1) 1) (integer) 0 2) (integer) 16383 3) 1) "" 2) (integer) 6379 3) "f1aeceb352401ce57acd432c68c60b359c00ef85" 4) (empty array) 127.0.0.1:6379> hello 3 1# "server" => "valkey" 2# "version" => "255.255.255" 3# "proto" => (integer) 3 4# "id" => (integer) 3 5# "mode" => "cluster" 6# "role" => "master" 7# "modules" => (empty array) 127.0.0.1:6379> cluster slots 1) 1) (integer) 0 2) (integer) 16383 3) 1) "" 2) (integer) 6379 3) "f1aeceb352401ce57acd432c68c60b359c00ef85" 4) (empty array) ``` RESP3 should get "empty hash" but get RESP2's "empty array" 3. we should use the original client's connect type, or lua/function and module would get wrong port: ``` $./valkey-cli --tls --insecure -p 6789 127.0.0.1:6789> config get port tls-port 1) "tls-port" 2) "6789" 3) "port" 4) "6379" 127.0.0.1:6789> cluster slots 1) 1) (integer) 0 2) (integer) 16383 3) 1) "" 2) (integer) 6789 3) "f1aeceb352401ce57acd432c68c60b359c00ef85" 4) (empty array) 127.0.0.1:6789> eval "return redis.call('cluster','slots')" 0 1) 1) (integer) 0 2) (integer) 16383 3) 1) "" 2) (integer) 6379 3) "f1aeceb352401ce57acd432c68c60b359c00ef85" 4) (empty array) ``` --------- Signed-off-by: zhaozhao.zz <zhaozhao.zz@alibaba-inc.com>
This PR adds a logic to cache
CLUSTER SLOTS
response for reduced latency and also update the cache when a change in the cluster is detected.Historically,
CLUSTER SLOTS
command was deprecated, however all the server clients have been usingCLUSTER SLOTS
and have not migrated toCLUSTER SHARDS
. In future this logic can be added to any other commands to improve the performance of the engine.To compare the performance gain this PR has to offer, I have ran benchmarks for 2 scenarios with 2 primaries in the cluster:
Complete Benchmark results:
0.044msec
to0.021msec
for100000
requests.BEST CASE unstable branch:
BEST CASE This PR:
4.026msec
to0.181msec
for1000
requests.WORST CASE unstable - Single Benchmark:
WORST CASE This PR - Single Benchmark:
This seemed like benckmark was the bottleneck for worst case.
So I also tried running 5 benchmarks for Worst case scenario with UNSTABLE and this PR:
All benchmark were around 25rps and
17.8msec
each and the CPU utilization of server went up to around 56% ,while for this PR
All benchmark rps was around 48rps and
0.28msec
each and the CPU utilization of server went down to just 12%Thats seems like 100% gain in RPS and 60X less latency and around 5X drop in CPU usage
for below images
placeholderkv-s
is the server andplaceholderkv-b
are the benchmarksWORST CASE scenario unstable - 5 Benchmarks:
CPU utilizations:
Benchmarks:
WORST CASE scenario PR - 5 Benchmarks:
CPU utilizations:
Benchmarks:
CONCLUSION: