Add rediscluster support #573

FranGM · 2021-03-08T18:05:32Z

Add a ClusterRedisClient next to the existing RedisClient, allowing us to connect to Redis clusters.

We also provide a MonitoredClusterRedisConnection and MonitoredClusterRedisPipeline as analogues for MonitoredRedisConnection and MonitoredRedisPipeline.

spladug

This looks great! Thank you! I've left a few small comments in there, but overall it looks like it'll do the trick nicely. A few blockers:

splitting up the files so we don't cause import issues for existing users
adding a page to the docs here: https://github.com/reddit/baseplate.py/tree/develop/docs/api/baseplate/clients (you can do make docs in dev to see the output)
adding an appropriate "extra" to setup.py to specify supported version ranges: https://github.com/reddit/baseplate.py/blob/develop/setup.py#L14
some lint failures (you can do make lint and make fmt in dev https://github.com/reddit/baseplate.py/blob/develop/CONTRIBUTING.md)

spladug · 2021-03-08T18:15:49Z

baseplate/clients/redis.py

@@ -4,6 +4,7 @@
 from typing import Optional

 import redis
+import rediscluster


This would make anyone using the current redis client suddenly have to install redis-py-cluster as well just to continue using the other client. Can you move the new cluster stuff into its own module in the clients folder? Like baseplate/clients/redis_cluster.py perhaps?

baseplate/clients/redis.py

spladug · 2021-03-08T18:20:08Z

baseplate/clients/redis.py

+
+    """
+
+    # TODO: Add all args below


spladug · 2021-03-08T18:21:04Z

docker-compose.yml

@@ -25,3 +28,29 @@ services:
    image: "redis:4.0.9"
  zookeeper:
    image: "zookeeper:3.4.10"
+  redis-cluster-node-0:


wow, this is a lot of stuff. do we actually need a three node cluster to test this out? or can we mock it more simply?

One idea would be to use

grokzen/redis-cluster

which gives us a Redis cluster with a single Docker dependency. It is not recommended for production use but perfect for testing environments. I will say, Redis cluster has idiosyncrasies that are worth having tests run against a real instance but that depends entirely on the tests.

we're not actually implementing a full redis cluster client here, thankfully, but just wrapping an existing one. do you think we need to have behavioral tests here in baseplate beyond ensuring that we generate spans?

There were a few cases where I was thinking more testing would be useful but ultimately any test I could think of would just end up testing the underlying redis-py-cluster library, so didn't add anything beyond what the redis client already has (I did end up using the redis-cluster container for the integration tests though, it was also pretty handy for manual testing)

spladug · 2021-03-08T18:21:21Z

tests/integration/redis_tests.py

@@ -6,9 +6,13 @@
 except ImportError:
    raise unittest.SkipTest("redis-py is not installed")

+try:


likewise on splitting up the redis/redis_cluster modules here please

Co-authored-by: Neil Williams <[email protected]>

FranGM · 2021-03-10T21:08:12Z

I think I've addressed all the comments. I've split redis into two clients and added a page to the docs.
After more testing I ended up needing to work around a quirk or two of the upstream library but nothing too major.

spladug

looks great! only blocker is the question about our default causing the infinite loop as well. otherwise, just some stylistic questions. thank you!

spladug · 2021-03-10T22:36:56Z

baseplate/clients/redis_cluster.py

+from typing import Any
+from typing import Dict
+
+import rediscluster  # type: ignore


is this because rediscluster isn't typed? if so, you can get rid of the pragmas here in code and pop something like this into setup.cfg: https://github.com/reddit/baseplate.py/blob/develop/setup.cfg#L72-L73

oh that's great , that's much cleaner

spladug · 2021-03-10T23:35:50Z

baseplate/clients/redis_cluster.py

+        nodes_in_slot = self.nodes.slots[slot]
+        if read_command:
+            random_index = random.randrange(1, len(nodes_in_slot))
+            return nodes_in_slot[random_index]
+
+        return nodes_in_slot[0]


Am I understanding right that the first node is always the primary and the rest are replicas? If so, would something like this make that a little more self-explanatory?

Suggested change

nodes_in_slot = self.nodes.slots[slot]

if read_command:

random_index = random.randrange(1, len(nodes_in_slot))

return nodes_in_slot[random_index]

return nodes_in_slot[0]

primary, *replicas = self.nodes.slots[slot]

if read_command:

return random.choice(replicas)

return primary

Edge case check: is there ever a situation where we'd have zero replicas?

actually it's definitely possible, yes, let me rework it so it can handle all cases

baseplate/clients/redis_cluster.py

spladug · 2021-03-10T23:41:10Z

tests/integration/redis_tests.py

@@ -8,7 +8,6 @@

 from baseplate.clients.redis import RedisClient
 from baseplate import Baseplate
-


Co-authored-by: Neil Williams <[email protected]>

spladug

LGTM!

spladug · 2021-03-30T18:33:18Z

baseplate/clients/redis_cluster.py

-        # Either this isn't a read command or there aren't any replicas
-        return primary
+        # This isn't a read command, so return the primary
+        return self.nodes.slots[slot]


does this not return all of them rather than just the primary?

ah yes, and I'm very annoyed at myself for not having added a test for that method. I'll fix it and look at adding a test.

For rediscluster we can't easily calculate the size of the pool since counting the number of items in the queue doesn't work (the queue is always full, just full of Nones if the queue is "empty"). We just decide to ignore that metric for now and just report the ones that are easily available to us.

This will allow us to take advantage of `read_from_replicas` support on pipelines when Grokzen/redis-py-cluster#450 is merged

Previous versions of redis-py-cluster don't accept the `read_from_replicas` argument to pipelines

Add a feature to the cluster redis client that can enable hot key tracking based on a configurable sample rate. When read and/or write commands access keys in Redis we will sample those commands and store the keys within a sorted set in Redis, which we can use to check relative frequency of accessing some keys compared to others.

… into rediscluster-support

FranGM · 2021-04-29T17:17:31Z

@spladug based on our testing I added a requested feature to the client which is the ability to track hot keys in the cluster.

Another change that's pending is that I need to bump the version of redis-py-cluster to 2.1.3 but need to wait until that is released first. We'll need that version since it will include Grokzen/redis-py-cluster#450

spladug

looks great! just a couple notes on the docs. once that's done, we can merge whenever that new release is cut upstream.

spladug · 2021-04-29T18:41:17Z

docs/api/baseplate/clients/redis_cluster.rst

+   How many connections have been established and are currently checked out and
+   being used.
+
+.. versionadded:: 2.1


can this go up at the top, like right before the Example heading? I've only put the versionadded stuff down here on some other clients where the runtime metrics were the thing that was new.

spladug · 2021-04-29T20:27:09Z

docs/api/baseplate/clients/redis_cluster.rst

@@ -0,0 +1,92 @@
+``baseplate.clients.redis_cluster``


should the hot key tracking stuff be mentioned somewhere in this page?

docs/api/baseplate/clients/redis_cluster.rst

Fran Garcia added 2 commits March 8, 2021 13:36

Add redisc cluster client support

7492d69

Add integration tests for ClusterRedisClient

e9b646a

FranGM requested a review from a team as a code owner March 8, 2021 18:05

remove unused import

6050bc3

spladug self-requested a review March 8, 2021 18:12

Fran Garcia added 3 commits March 8, 2021 18:17

remove unnneeded comment

8d5d40b

remove unused import

cb95feb

disable pylint warning

9655d17

spladug suggested changes Mar 8, 2021

View reviewed changes

FranGM and others added 3 commits March 8, 2021 18:32

Update baseplate/clients/redis.py

6214d10

Co-authored-by: Neil Williams <[email protected]>

Update baseplate/clients/redis.py

78dc569

Co-authored-by: Neil Williams <[email protected]>

address comments

1765def

FranGM requested a review from spladug March 10, 2021 21:08

set version

ffdecf9

spladug suggested changes Mar 10, 2021

View reviewed changes

FranGM and others added 2 commits March 10, 2021 23:53

Update baseplate/clients/redis_cluster.py

9570abf

Co-authored-by: Neil Williams <[email protected]>

address comments (take 2)

df35f03

FranGM requested a review from spladug March 11, 2021 00:13

spladug approved these changes Mar 11, 2021

View reviewed changes

When read_from_replicas is enabled also read from master

94914ae

spladug reviewed Mar 30, 2021

View reviewed changes

FranGM added 4 commits March 30, 2021 22:59

Return only one node on get_node_by_slot

191ed5d

fix documentation lint issues

c78dcf2

change some client defaults

8db95aa

add more tests

024adff

FranGM requested a review from spladug March 30, 2021 23:32

spladug approved these changes Mar 30, 2021

View reviewed changes

Fran Garcia and others added 2 commits April 6, 2021 18:57

pass read_from_replicas arg when creating pipeline

aeb416d

This will allow us to take advantage of `read_from_replicas` support on pipelines when Grokzen/redis-py-cluster#450 is merged

FranGM and others added 3 commits April 20, 2021 14:36

bump redis-py-cluster version

2c099df

Previous versions of redis-py-cluster don't accept the `read_from_replicas` argument to pipelines

Merge branch 'rediscluster-support' of github.com:FranGM/baseplate.py…

35839ad

… into rediscluster-support

spladug approved these changes Apr 29, 2021

View reviewed changes

FranGM and others added 5 commits May 2, 2021 22:20

Allow to configure max_connections_per_node

4255cf2

Update to current redis-py-cluster version

972f82d

Update requirements-transitive.txt

96054f4

Add hot key tracking to docs

c35f976

Update setup.py

44ecda5

spladug approved these changes May 19, 2021

View reviewed changes

docs/api/baseplate/clients/redis_cluster.rst Outdated Show resolved Hide resolved

Update docs/api/baseplate/clients/redis_cluster.rst

99b4ffd

spladug merged commit fa3487b into reddit:develop May 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add rediscluster support #573

Add rediscluster support #573

FranGM commented Mar 8, 2021

spladug left a comment •

edited

Loading

spladug Mar 8, 2021

spladug Mar 8, 2021

spladug Mar 8, 2021

konradreiche Mar 9, 2021

spladug Mar 9, 2021

FranGM Mar 10, 2021

spladug Mar 8, 2021

FranGM commented Mar 10, 2021

spladug left a comment

spladug Mar 10, 2021

FranGM Mar 11, 2021

spladug Mar 10, 2021

FranGM Mar 10, 2021

spladug Mar 10, 2021

spladug left a comment

spladug Mar 30, 2021

FranGM Mar 30, 2021

FranGM commented Apr 29, 2021

spladug left a comment

spladug Apr 29, 2021

spladug Apr 29, 2021

		@@ -8,7 +8,6 @@

		from baseplate.clients.redis import RedisClient
		from baseplate import Baseplate

Add rediscluster support #573

Add rediscluster support #573

Conversation

FranGM commented Mar 8, 2021

spladug left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranGM commented Mar 10, 2021

spladug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spladug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FranGM commented Apr 29, 2021

spladug left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spladug left a comment •

edited

Loading