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

Counting reply_channels in a Group #388

Closed
masterrex opened this Issue Oct 8, 2016 · 3 comments

Comments

Projects
None yet
2 participants
@masterrex

masterrex commented Oct 8, 2016

Hi folks, this is a pseudo-enhancement request discussion. If there's a better forum for this, please advise.

I've been tinkering with the chat use case. One of the first things I've tried to do is determine how many users ("reply_channels") are in a "room" (Group) at a time. In my head, calling myroom.websocket_group.count() would make sense.

Currently, retrieving this count requires getting the RedisChannelLayer instance and asking for all of the key/value pairs from the Group zset, then getting the length of the returned list:

len(Room.objects.first().websocket_group.channel_layer.channel_layer.group_channels('room-1'))

This also works:

from channels import channel_layers
len(channel_layers.backends['default'].channel_layer.group_channels('room-1'))

It feels dirty to retrieve all the data when you just need the count. When using redis as a backend, we can issue the zcard command to retrieve row count for a given zset, i.e:

zcard asgi::group:room-1

Redis Desktop Manager Screenshot

My knowledge/research falls short at this point. Could we extend Group class this enhancement be generic enough to implement for all backends? The hacky way can be implemented easily:

class Group(object):
    """
    A group of channels that can be messaged at once, and that expire out
    of the group after an expiry time (keep re-adding to keep them in).
    """
    def __len__(self):
        return len(self.channel_layer.group_channels(self.name))

    def count(self):
        return self.__len__()

Update - we can use the native connection in a backend-specific (asgi_redis.core.RedisChannelLayer) fashion:

from channels import channel_layers
rcl = channel_layers.backends['default'].channel_layer.group_channels.__self__
rcl.connection(0).zcard('asgi::group:room-1')
@masterrex

This comment has been minimized.

Show comment
Hide comment
@masterrex

masterrex Oct 8, 2016

I added a PR with the "hacky" solution that should be backend agnostic: #389

I added a PR with the "hacky" solution that should be backend agnostic: #389

@andrewgodwin

This comment has been minimized.

Show comment
Hide comment
@andrewgodwin

andrewgodwin Oct 10, 2016

Member

This has been possible from day one, but I want to ask a different question: why do you think this would be useful?

Basically, every point at which the API gets extended is one more point where backends are not free to optimise how they wish, and providing accurate lengths is one of these things. I added group_channels after it was pointed out it was needed for group merges, and it fills in for a length where needed.

It's deliberately not exposed via the Channels API though as it doesn't perform well and I don't want to invest time and insertion speed into making it do so without a compelling argument for having lengths. In particular, you cannot use group lengths for presence, as channels may exist in a group for up to 24 hours (by default) before the last cleanup gets them if they disconnect uncleanly.

If you have a good set of scenarios that channels can fulfill by adding group size that it can do accurately and in a way that won't mislead people, please share! That's why this hasn't been in here until now. If not, then I'm going to have to say no.

Member

andrewgodwin commented Oct 10, 2016

This has been possible from day one, but I want to ask a different question: why do you think this would be useful?

Basically, every point at which the API gets extended is one more point where backends are not free to optimise how they wish, and providing accurate lengths is one of these things. I added group_channels after it was pointed out it was needed for group merges, and it fills in for a length where needed.

It's deliberately not exposed via the Channels API though as it doesn't perform well and I don't want to invest time and insertion speed into making it do so without a compelling argument for having lengths. In particular, you cannot use group lengths for presence, as channels may exist in a group for up to 24 hours (by default) before the last cleanup gets them if they disconnect uncleanly.

If you have a good set of scenarios that channels can fulfill by adding group size that it can do accurately and in a way that won't mislead people, please share! That's why this hasn't been in here until now. If not, then I'm going to have to say no.

@andrewgodwin

This comment has been minimized.

Show comment
Hide comment
@andrewgodwin

andrewgodwin Apr 6, 2017

Member

Closing, as I don't believe this would be useful (and it is not possible to implement on some backends), and no good reasons have been presented.

Member

andrewgodwin commented Apr 6, 2017

Closing, as I don't believe this would be useful (and it is not possible to implement on some backends), and no good reasons have been presented.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment