Internet Simulator: Test Redis shard selection #17507
Conversation
shared/middleware/net_sim_api.rb
Outdated
| @@ -420,7 +393,7 @@ def redis_group_for_shard(shard_id) | |||
| # | |||
| # @return [Hash<'master':String, 'read_replicas':String[]>[]] | |||
| def redis_groups | |||
| CDO.netsim_redis_groups || [{'master': 'redis://localhost:6379'}] | |||
| CDO.netsim_redis_groups || [{'master' => 'redis://localhost:6379'}] | |||
There was a problem hiding this comment.
I messed up Ruby syntax and this was working accidentally. I need the key to be a string because that's the format we get back from our configuration. Writing this as {'master': '...'} makes the key a symbol. Everything still worked because when we eventually asked for config['master'] and got nil that's what we passed to the Redis constructor, which in turn defaulted to localhost and port 6379.
There was a problem hiding this comment.
Should this url be in a constant?
| @@ -5,11 +5,11 @@ | |||
| # Caution: This test is destructive, clears the whole Redis instance between | |||
| # tests, so be careful when using real redis. | |||
|
|
|||
| require_relative 'test_helper' | |||
| require_relative '../test_helper' | |||
There was a problem hiding this comment.
While I was in here,decided to go ahead and move this test into the middleware directory to reflect the position of the source file it tests.
| # | ||
| # @param [Proc] new_redis_proc (optional) is provided as a hook to allow | ||
| # us to construct fake Redis instances in test. | ||
| def initialize(shards, new_redis_proc = proc {|url| Redis.new(url: url)}) |
There was a problem hiding this comment.
note: you can also use the "stab operator" lambda syntax new_redis_proc = ->(url) {Redis.new(url: url)}
Either way is fine
There was a problem hiding this comment.
Since procs and lambdas are slightly different and I'm not super familiar with those differences, I'm going to leave this as-is for now.
| def initialize(shards, new_redis_proc = proc {|url| Redis.new(url: url)}) | ||
| @new_redis_proc = new_redis_proc | ||
| @ring = ConsistentHashing::Ring.new | ||
| shards.each {|shard_config| @ring.add shard_config} |
There was a problem hiding this comment.
What happens if shards is empty (say missing / empty configuration)? Should we check here and maybe log an error? Will it continue to work, or fail later?
There was a problem hiding this comment.
You'll get an ArgumentError if you don't pass a shards argument. I suppose we should also raise if you pass an empty array, since we must have at least one endpoint.
shared/middleware/net_sim_api.rb
Outdated
| @@ -420,7 +393,7 @@ def redis_group_for_shard(shard_id) | |||
| # | |||
| # @return [Hash<'master':String, 'read_replicas':String[]>[]] | |||
| def redis_groups | |||
| CDO.netsim_redis_groups || [{'master': 'redis://localhost:6379'}] | |||
| CDO.netsim_redis_groups || [{'master' => 'redis://localhost:6379'}] | |||
There was a problem hiding this comment.
Should this url be in a constant?
| ) | ||
|
|
||
| # Shard selection is deterministic, so we just found appropriate keys | ||
| # for this test without pinning any randomness. |
There was a problem hiding this comment.
Pretty convenient that they all turned out to be coherent "a ___ shard key" statements 👍
There was a problem hiding this comment.
😁 These took some trial-and-error.
| assert_equal 'redis://replica1_1', client.slaves[0].url_according_to_test | ||
| assert_equal 'redis://replica1_2', client.slaves[1].url_according_to_test | ||
|
|
||
| client = factory.client_for_key('second shard') |
As a follow-up to #17436 I've extracted a
ShardedRedisFactorythat encapsulates the logic for building a configured Redis client given a Redis cluster configuration and a shard key. It's fairly simple, just mashing together theconsistent-hashing,redisandredis-slave-readgems with some knowledge of our configuration structure.Unit testing was still a little tricky because these libraries are fairly locked-down and I wasn't about to set up replication groups with FakeRedis, but by injecting a Redis constructor function I can at least verify that a particular configuration and shard key results in an expected
HiRedisinstance wrapping Redis clients with expected URLs.