feat(redis): support cluster publishing pipelines - #3096
Conversation
|
The Redis and Redis Cluster integration jobs passed, including the new transaction tests. The Kafka job finished with 359 passed and one failure in That case did not receive its message within the existing three-second wait; aiokafka logged that the generated topic was not found in cluster metadata. The same inherited test passed later in the job as Could a maintainer rerun the failed jobs in this run? I only have read access to the upstream repository, so I cannot trigger the rerun. |
IvanKirpichnikov
left a comment
There was a problem hiding this comment.
In FastStream, we support three types of Redis brokers:
- Default Redis
- Redis Cluster
- Redis Sentinel
For regular Redis and Redis Sentinel, the pipeline type is redis.asyncio.client.Pipeline, while for Redis Cluster it is redis.asyncio.cluster.ClusterPipeline.
We need to add the correct type hints for the pipeline argument in the following methods:
publishrequestpublish_batch- e.g...
We also need to add overloads for these methods so that the return type depends on whether a pipeline is passed.
The expected behavior is:
- Without pipeline → keep the existing return type.
- With pipeline → return the corresponding broker pipeline type, as in redis-py.
In other words, the overloads should reflect the following behavior:
- For regular Redis and Sentinel:
redis.asyncio.client.Pipeline - For Redis Cluster:
redis.asyncio.cluster.ClusterPipeline
Return the matching Redis pipeline from publishing overloads and keep RedisClusterBroker restricted to ClusterPipeline. Request remains pipeline-free because request-reply must publish immediately before awaiting a response.
c09a056 to
679e515
Compare
|
Addressed in
I intentionally kept @IvanKirpichnikov, could you take another look? |
| pipeline: Optional["Pipeline[bytes] | ClusterPipeline[bytes]"] = None, | ||
| ) -> "int | bytes | Pipeline[bytes] | ClusterPipeline[bytes]": |
There was a problem hiding this comment.
only Pipeline. It can only be used with ClusterRedis
|
|
||
| result: int | bytes = await super()._basic_publish( | ||
| result: ( | ||
| int | bytes | Pipeline[bytes] | ClusterPipeline[bytes] |
There was a problem hiding this comment.
only Pipeline. It can only be used with ClusterRedis
| pipeline: Optional["Pipeline[bytes] | ClusterPipeline[bytes]"] = None, | ||
| ) -> "int | Pipeline[bytes] | ClusterPipeline[bytes]": |
There was a problem hiding this comment.
only Pipeline. It can only be used with ClusterRedis
|
|
||
| result: int = await self._basic_publish_batch( | ||
| result: ( | ||
| int | Pipeline[bytes] | ClusterPipeline[bytes] |
There was a problem hiding this comment.
only Pipeline. It can only be used with ClusterRedis
|
|
||
| A cross-slot transaction must fail without publishing outside it. | ||
| """ | ||
| from redis.exceptions import CrossSlotTransactionError |
There was a problem hiding this comment.
Make the import at the module level.
| broker = self.get_broker() | ||
| counter = f"{{first}}:{queue}:count" | ||
| stream = f"{{second}}:{queue}:stream" | ||
|
|
||
| async with broker: | ||
| client = await broker.connect() | ||
| try: | ||
| async with client.pipeline(transaction=True) as pipe: | ||
| pipe.incr(counter) | ||
| await broker.publish("one", stream=stream, pipeline=pipe) | ||
| with pytest.raises(CrossSlotTransactionError): | ||
| await pipe.execute() | ||
|
|
||
| assert await client.get(counter) is None | ||
| assert await client.xlen(stream) == 0 | ||
| finally: | ||
| await client.delete(counter) | ||
| await client.delete(stream) |
There was a problem hiding this comment.
broker = self.get_broker()
counter = f"{{first}}:{queue}:count"
stream = f"{{second}}:{queue}:stream"
async with self.patch_broker(broker):
client = broker.config.broker_config.connection.client
async with client.pipeline(transaction=True) as pipe:
pipe.incr(counter)
await broker.publish("one", stream=stream, pipeline=pipe)
with pytest.raises(CrossSlotTransactionError):
await pipe.execute()
assert await client.get(counter) is None
assert await client.xlen(stream) == 0| maxlen=cmd.maxlen, | ||
| ) | ||
| else: | ||
| raise UnreachablePathError |
There was a problem hiding this comment.
maybe use assert_never(cmd.destination_type)
| broker = self.get_broker() | ||
| counter = f"{{{queue}}}:count" | ||
| stream = f"{{{queue}}}:stream" | ||
| publisher = broker.publisher(stream=stream) | ||
|
|
||
| async with broker: | ||
| client = await broker.connect() | ||
| try: | ||
| async with client.pipeline(transaction=True) as pipe: | ||
| await pipe.watch(counter) | ||
| result = await broker.publish( | ||
| "immediate", stream=stream, pipeline=pipe | ||
| ) | ||
| assert isinstance(result, bytes) | ||
| assert await client.xlen(stream) == 1 | ||
|
|
||
| pipe.multi() | ||
| pipe.incr(counter) | ||
| await publisher.publish("queued", pipeline=pipe) | ||
| assert await client.get(counter) is None | ||
| assert await client.xlen(stream) == 1 | ||
|
|
||
| results = await pipe.execute() | ||
|
|
||
| assert results[0] == 1 | ||
| assert len(results) == 2 | ||
| assert await client.get(counter) == b"1" | ||
| entries = await client.xrange(stream) | ||
| assert [ | ||
| broker.message_format.parse(fields[b"__data__"])[0] | ||
| for _, fields in entries | ||
| ] == [b"immediate", b"queued"] | ||
| finally: | ||
| await client.delete(counter, stream) |
There was a problem hiding this comment.
broker = self.get_broker()
counter = f"{{{queue}}}:count"
stream = f"{{{queue}}}:stream"
publisher = broker.publisher(stream=stream)
async with self.patch_broker(broker):
client = broker.config.broker_config.connection.client
async with client.pipeline(transaction=True) as pipe:
await pipe.watch(counter)
result = await broker.publish(
"immediate", stream=stream, pipeline=pipe
)
assert isinstance(result, bytes)
assert await client.xlen(stream) == 1
pipe.multi()
pipe.incr(counter)
await publisher.publish("queued", pipeline=pipe)
assert await client.get(counter) is None
assert await client.xlen(stream) == 1
results = await pipe.execute()
assert results[0] == 1
assert len(results) == 2
assert await client.get(counter) == b"1"
entries = await client.xrange(stream)
assert [
broker.message_format.parse(fields[b"__data__"])[0]
for _, fields in entries
] == [b"immediate", b"queued"]| broker = self.get_broker() | ||
| counter = f"{{{queue}}}:count" | ||
| destination = f"{{{queue}}}:list" | ||
| publisher = broker.publisher(list=ListSub(destination, batch=True)) | ||
|
|
||
| async with broker: | ||
| client = await broker.connect() | ||
| try: | ||
| async with client.pipeline(transaction=True) as pipe: | ||
| pipe.incr(counter) | ||
| await broker.publish_batch( | ||
| "one", "two", list=destination, pipeline=pipe | ||
| ) | ||
| await publisher.publish("three", "four", pipeline=pipe) | ||
|
|
||
| assert await client.exists(counter, destination) == 0 | ||
| assert await pipe.execute() == [1, 2, 4] | ||
|
|
||
| assert await client.get(counter) == b"1" | ||
| messages = await client.lrange(destination, 0, -1) | ||
| assert [broker.message_format.parse(msg)[0] for msg in messages] == [ | ||
| b"one", | ||
| b"two", | ||
| b"three", | ||
| b"four", | ||
| ] | ||
| finally: | ||
| await client.delete(counter, destination) |
There was a problem hiding this comment.
broker = self.get_broker()
counter = f"{{{queue}}}:count"
destination = f"{{{queue}}}:list"
publisher = broker.publisher(list=ListSub(destination, batch=True))
async with self.patch_broker(broker):
client = broker.config.broker_config.connection.client
async with client.pipeline(transaction=True) as pipe:
pipe.incr(counter)
await broker.publish_batch(
"one", "two", list=destination, pipeline=pipe
)
await publisher.publish("three", "four", pipeline=pipe)
assert await client.exists(counter, destination) == 0
assert await pipe.execute() == [1, 2, 4]
assert await client.get(counter) == b"1"
messages = await client.lrange(destination, 0, -1)
assert [broker.message_format.parse(msg)[0] for msg in messages] == [
b"one",
b"two",
b"three",
b"four",
]| broker = self.get_broker() | ||
| counter = f"{{{queue}}}:count" | ||
| stream = f"{{{queue}}}:stream" | ||
| publisher = broker.publisher(stream=stream) | ||
|
|
||
| async with broker: | ||
| client = await broker.connect() | ||
| try: | ||
| async with client.pipeline(transaction=True) as pipe: | ||
| pipe.incr(counter) | ||
| await broker.publish( | ||
| "one", | ||
| stream=stream, | ||
| correlation_id=queue, | ||
| headers={"source": "broker"}, | ||
| pipeline=pipe, | ||
| ) | ||
| await publisher.publish("two", pipeline=pipe) | ||
|
|
||
| assert await client.exists(counter, stream) == 0 | ||
| results = await pipe.execute() | ||
|
|
||
| assert results[0] == 1 | ||
| assert len(results) == 3 | ||
| assert await client.get(counter) == b"1" | ||
| entries = await client.xrange(stream) | ||
| messages = [ | ||
| broker.message_format.parse(fields[b"__data__"]) | ||
| for _, fields in entries | ||
| ] | ||
| assert [body for body, _ in messages] == [b"one", b"two"] | ||
| assert messages[0][1]["correlation_id"] == queue | ||
| assert messages[0][1]["source"] == "broker" | ||
| finally: | ||
| await client.delete(counter, stream) |
There was a problem hiding this comment.
broker = self.get_broker()
counter = f"{{{queue}}}:count"
stream = f"{{{queue}}}:stream"
publisher = broker.publisher(stream=stream)
async with self.patch_broker(broker):
client = broker.config.broker_config.connection.client
async with client.pipeline(transaction=True) as pipe:
pipe.incr(counter)
await broker.publish(
"one",
stream=stream,
correlation_id=queue,
headers={"source": "broker"},
pipeline=pipe,
)
await publisher.publish("two", pipeline=pipe)
assert await client.exists(counter, stream) == 0
results = await pipe.execute()
assert results[0] == 1
assert len(results) == 3
assert await client.get(counter) == b"1"
entries = await client.xrange(stream)
messages = [
broker.message_format.parse(fields[b"__data__"])
for _, fields in entries
]
assert [body for body, _ in messages] == [b"one", b"two"]
assert messages[0][1]["correlation_id"] == queue
assert messages[0][1]["source"] == "broker"
Description
Fixes #3044.
RedisClusterBrokercurrently ignorespipeline=, so a state update and a publication cannot share a transaction. Simply forwarding it is not enough: awaiting theClusterPipelinereturned by a queued command reinitializes the pipeline and clears its queued commands.This change forwards the supplied pipeline through broker and publisher calls for lists, streams, and list batches. Queued calls return the pipeline without awaiting it; commands issued after
WATCHbut beforeMULTIstill execute immediately. Publishing without a pipeline keeps its existing behavior, including automatic connection for cluster batch publishing.Channel
PUBLISHremains blocked by redis-py in cluster pipelines. There is no immediate-publish fallback. Atomic transactions require redis-py 6.2.0 or later and keys in the same hash slot; ordinary pipelines are not atomic.Type of change
Validation
127.0.0.1locally. Windows/WSL'slocalhostIPv6 fallback exceeded existing timeouts; the same RPC timeout reproduced on unchanged main. No production code or test timeout was changed for this environment issue.justwrappers were checked through their underlying commands.Checklist