Support readonly pipelines in MultiStore
What does this MR do and why?
This MR adds support for readonly pipelines in MultiStore. This avoids performance regression when migrating between 2 Redis if there are pipeline/multi only containing read operations.
See gitlab-com/gl-infra/scalability#2016 (comment 1431868698)
Other ideas while drafting the MR
- Introducing a pipelined_read function in MultiStore. Decided against it as it would not work cleanly with
CrossSlot::Pipeline
- Directly sending pipeline to
ClusterCache
. Decided against it as (1) lots of feature-flag checks which adds complexity to the code and (2)RedisStorePatch
affects allRedisCacheStore
(Gitlab::Redis::RepositoryCache
andGitlab::Redis::FeatureFlag
are not using MultiStore), doing so would complicate the patch.
This MR implements an existing pattern seen in RedisClusterValidator
.
Screenshots or screen recordings
Screenshots are required for UI changes, and strongly recommended for all other merge requests.
Before | After |
---|---|
How to set up and validate locally
Pre-requisite
- Set GDK to run a Redis Cluster (https://gitlab.com/gitlab-org/gitlab-development-kit/-/blob/main/data/announcements/0004_redis_cluster_support.yml)
- Update
redis.yml
- Enable both
use_primary_store_as_default_for_cache
anduse_primary_store_as_default_for_cache
feature flag
development:
cluster_cache:
cluster:
- redis://localhost:6001
On master branch
On gdk rails console
. We can see that a pipeline of read commands will go to both Cache and ClusterCache.
[23] pry(main)> RequestStore.begin!
=> true
[24] pry(main)> ::RequestStore.active?
=> true
[25] pry(main)> Gitlab::Redis::Cache.with do |c|
[25] pry(main)* c.pipelined do |p|
[25] pry(main)* p.get('x')
[25] pry(main)* end
[25] pry(main)* end
=> [nil]
[26] pry(main)> Gitlab::Instrumentation::Redis::Cache.get_request_count
=> 1
[27] pry(main)> Gitlab::Instrumentation::Redis::ClusterCache.get_request_count
=> 1
On this branch
We still see that a pipeline of read commands will go to both Cache and ClusterCache.
[1] pry(main)> ::RequestStore.active?
=> false
[2] pry(main)> RequestStore.begin!
=> true
[3] pry(main)> ::RequestStore.active?
=> true
[4] pry(main)> Gitlab::Redis::Cache.with do |c|
[4] pry(main)* c.pipelined do |p|
[4] pry(main)* p.get('x')
[4] pry(main)* end
[4] pry(main)* end
=> [nil]
[5] pry(main)> Gitlab::Instrumentation::Redis::Cache.get_request_count
=> 1
[6] pry(main)> Gitlab::Instrumentation::Redis::ClusterCache.get_request_count
=> 1
But using the read-only pipeline, the command is sent to ClusterCache only.
[8] pry(main)> RequestStore.clear!
=> {}
[9] pry(main)> RequestStore.begin!
=> true
...
[12] pry(main)> Gitlab::Redis::Cache.with do |c|
[12] pry(main)* Gitlab::Redis::MultiStore.with_readonly_pipeline do
[12] pry(main)* c.pipelined do |p|
[12] pry(main)* p.get('x')
[12] pry(main)* end
[12] pry(main)* end
[12] pry(main)* end
=> ["1"]
[13] pry(main)> Gitlab::Instrumentation::Redis::ClusterCache.get_request_count
=> 1
[14] pry(main)> Gitlab::Instrumentation::Redis::Cache.get_request_count
=> 0
MR acceptance checklist
This checklist encourages us to confirm any changes have been analyzed to reduce risks in quality, performance, reliability, security, and maintainability.
-
I have evaluated the MR acceptance checklist for this MR.