-
Notifications
You must be signed in to change notification settings - Fork 175
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
Add rediscluster support #573
Changes from 8 commits
7492d69
e9b646a
6050bc3
8d5d40b
cb95feb
9655d17
6214d10
78dc569
1765def
ffdecf9
9570abf
df35f03
94914ae
191ed5d
c78dcf2
8db95aa
024adff
b904a82
aeb416d
2c099df
1ee4bdc
35839ad
4255cf2
972f82d
96054f4
c35f976
44ecda5
99b4ffd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -13,6 +13,9 @@ services: | |
- "memcached" | ||
- "redis" | ||
- "zookeeper" | ||
- "redis-cluster-node-0" | ||
- "redis-cluster-node-1" | ||
- "redis-cluster-node-2" | ||
|
||
cassandra: | ||
image: "cassandra:3.11" | ||
|
@@ -25,3 +28,29 @@ services: | |
image: "redis:4.0.9" | ||
zookeeper: | ||
image: "zookeeper:3.4.10" | ||
redis-cluster-node-0: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. One idea would be to use
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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
image: docker.io/bitnami/redis-cluster:6.2-debian-10 | ||
environment: | ||
- 'ALLOW_EMPTY_PASSWORD=yes' | ||
- 'REDIS_NODES=redis-cluster-node-0 redis-cluster-node-1 redis-cluster-node-2' | ||
redis-cluster-node-1: | ||
image: docker.io/bitnami/redis-cluster:6.2-debian-10 | ||
environment: | ||
- 'ALLOW_EMPTY_PASSWORD=yes' | ||
- 'REDIS_NODES=redis-cluster-node-0 redis-cluster-node-1 redis-cluster-node-2' | ||
redis-cluster-node-2: | ||
image: docker.io/bitnami/redis-cluster:6.2-debian-10 | ||
environment: | ||
- 'ALLOW_EMPTY_PASSWORD=yes' | ||
- 'REDIS_NODES=redis-cluster-node-0 redis-cluster-node-1 redis-cluster-node-2' | ||
redis-cluster-init: | ||
image: docker.io/bitnami/redis-cluster:6.2-debian-10 | ||
depends_on: | ||
- redis-cluster-node-0 | ||
- redis-cluster-node-1 | ||
- redis-cluster-node-2 | ||
environment: | ||
- 'REDISCLI_AUTH=' | ||
- 'REDIS_CLUSTER_REPLICAS=1' | ||
- 'REDIS_NODES=redis-cluster-node-0 redis-cluster-node-1 redis-cluster-node-2' | ||
- 'REDIS_CLUSTER_CREATOR=yes' |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,6 @@ | |
|
||
from baseplate.clients.redis import RedisClient | ||
from baseplate import Baseplate | ||
|
||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gasp! |
||
from . import TestBaseplateObserver, get_endpoint_or_skip_container | ||
|
||
from baseplate.clients.redis import MessageQueue | ||
|
@@ -79,6 +78,67 @@ def test_pipeline(self): | |
self.assertIsNone(span_observer.on_finish_exc_info) | ||
|
||
|
||
class RedisClusterIntegrationTests(unittest.TestCase): | ||
def setUp(self): | ||
self.baseplate_observer = TestBaseplateObserver() | ||
|
||
baseplate = Baseplate({"rediscluster.url": f"redis://{redis_endpoint}/0"}) | ||
baseplate.register(self.baseplate_observer) | ||
baseplate.configure_context({"rediscluster": RedisClient()}) | ||
|
||
self.context = baseplate.make_context_object() | ||
self.server_span = baseplate.make_server_span(self.context, "test") | ||
|
||
def test_simple_command(self): | ||
with self.server_span: | ||
result = self.context.rediscluster.ping() | ||
|
||
self.assertTrue(result) | ||
|
||
server_span_observer = self.baseplate_observer.get_only_child() | ||
span_observer = server_span_observer.get_only_child() | ||
self.assertEqual(span_observer.span.name, "rediscluster.PING") | ||
self.assertTrue(span_observer.on_start_called) | ||
self.assertTrue(span_observer.on_finish_called) | ||
self.assertIsNone(span_observer.on_finish_exc_info) | ||
|
||
def test_error(self): | ||
with self.server_span: | ||
with self.assertRaises(redis.ResponseError): | ||
self.context.rediscluster.execute_command("crazycommand") | ||
|
||
server_span_observer = self.baseplate_observer.get_only_child() | ||
span_observer = server_span_observer.get_only_child() | ||
self.assertTrue(span_observer.on_start_called) | ||
self.assertTrue(span_observer.on_finish_called) | ||
self.assertIsNotNone(span_observer.on_finish_exc_info) | ||
|
||
def test_lock(self): | ||
with self.server_span: | ||
with self.context.rediscluster.lock("foo"): | ||
pass | ||
|
||
server_span_observer = self.baseplate_observer.get_only_child() | ||
|
||
self.assertGreater(len(server_span_observer.children), 0) | ||
for span_observer in server_span_observer.children: | ||
self.assertTrue(span_observer.on_start_called) | ||
self.assertTrue(span_observer.on_finish_called) | ||
|
||
def test_pipeline(self): | ||
with self.server_span: | ||
with self.context.rediscluster.pipeline("foo") as pipeline: | ||
pipeline.ping() | ||
pipeline.execute() | ||
|
||
server_span_observer = self.baseplate_observer.get_only_child() | ||
span_observer = server_span_observer.get_only_child() | ||
self.assertEqual(span_observer.span.name, "rediscluster.pipeline_foo") | ||
self.assertTrue(span_observer.on_start_called) | ||
self.assertTrue(span_observer.on_finish_called) | ||
self.assertIsNone(span_observer.on_finish_exc_info) | ||
|
||
|
||
class RedisMessageQueueTests(unittest.TestCase): | ||
qname = "redisTestQueue" | ||
|
||
|
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 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? Likebaseplate/clients/redis_cluster.py
perhaps?