Allow sending multiple messages in bulk #400
Conversation
olzhasar
force-pushed
the
bulk-sending
branch
2 times, most recently
from
5fa875f
to
15b50ae
Compare
There was a problem hiding this comment.
@carltongibson: when you have a moment could you take a look at this PR? I think it's ready for you
channels_redis/core.py
Outdated
| """ | ||
| Convert a passed message arg to a tuple of messages. | ||
| """ | ||
| if not isinstance(message, dict) and hasattr(message, "__iter__"): |
There was a problem hiding this comment.
Should we use isinstance(message, collections.abc.Iterable here instead? I think that would make this a bit more readable
There was a problem hiding this comment.
Indeed, python dict is an instance of collections.abc.Iterable
There was a problem hiding this comment.
I have two queries.
- Are we sure about changing the signature of the existing send, rather than adding a new method here? Like, we don't use type hints yet, but think through the change there... from single message type to a union of that and an iterable of such... — does that look like a change we'd make?
- Do we need to update the docs and spec for channel layers here? https://channels.readthedocs.io/en/latest/channel_layer_spec.html — I'd think we do.
Ref 1, I'm slightly wary of adjusting the existing method, as we've had regressions in the Lua scripting before, which are hard to anticipate outside of production.
channels_redis/core.py
Outdated
| """ | ||
| Convert a passed message arg to a tuple of messages. | ||
| """ | ||
| if not isinstance(message, dict) and hasattr(message, "__iter__"): |
channels_redis/core.py
Outdated
| def _parse_messages(self, message): | ||
| """ | ||
| Convert a passed message arg to a tuple of messages. | ||
| """ | ||
| if not isinstance(message, dict) and hasattr(message, "__iter__"): | ||
| return tuple(message) | ||
| return (message,) |
There was a problem hiding this comment.
Seems a shame to consume the iterator only to loop over it again in the for message in messages loop. 🤔
I initially thought the same way and used two separate methods, but @bigfootjon's comment made me change my mind on that. I agreed cause I thought that typing is out of scope for everything django-related 😁
This is a legit concern. I'm actually okay with closing this PR if risks outweigh possible benefits from this feature. For my own use case, I've already made a workaround in place :) |
|
I think the benefits are there so I'd like to see this landed, but if @carltongibson would rather be conservative here I'll defer to his wisdom. @olzhasar would you mind undoing my recommendation and split up each method? Additionally, when you split things up please split the Lua script as well to make sure we don't regress |
|
("Wisdom" here means only "been bitten by more innocent looking things enough times to be wary" 😅) |
olzhasar
force-pushed
the
bulk-sending
branch
3 times, most recently
from
f2cec18
to
cbe1081
Compare
I've updated the PR splitting the methods as discussed, and preserving the old lua script for |
olzhasar
force-pushed
the
bulk-sending
branch
from
cbe1081
to
13debf7
Compare
Hey, The first one is a great catch, fixed that. As for the second thing, it generally can work either way, but I believe the current version leaves more room for future signature changes, e.g. introducing additional positional arguments. |
This is a draft implementation for #399
It adds support for sending multiple messages to the core layer only. It probably does not make much sense to add that to the pubsub layer, cause the underlying redis publish command only supports a single message.
I had to add an additional argument to the lua script for group sending, I hope that's not a big deal. But let me know if it is, I guess we can make the script dynamic.