search

larvalabs / breaker

Chat rooms for subreddits.
http://www.breakerapp.com
20 stars 3 forks source link

Active rooms list #23

Closed KenavR closed 8 years ago

KenavR commented 8 years ago

Active rooms - #3

Added a list to the sidebar which displays the most active rooms. The "activeness" is defined by the number of users that posted in the last month. I thought that would be better than currently active users, since people are looking for rooms that are in general more active instead of rooms they could chat right now. If you think that is not a good criteria it is just a matter of changing the sql statements.

Here is the SQL statement, I changed it a bit since I posted it in the issue.

SELECT id, name, displayName, iconUrl, activeUsers, rank
FROM (
    SELECT cr.id AS id, cr.name AS name, cr.displayname AS displayName, cr.iconurl AS iconUrl, COUNT(DISTINCT user_id) AS activeUsers, RANK() OVER (ORDER BY COUNT(DISTINCT user_id) DESC, name) as rank 
    FROM chatroom cr
    LEFT JOIN message m on cr.id = m.room_id
    WHERE m.createdate > (current_timestamp - interval '30 days')
    GROUP BY cr.id, cr.name, cr.displayname, cr.iconurl 
    ORDER BY activeUsers DESC
) AS ranked
WHERE name <> 'breakerapp' AND id NOT IN (
    SELECT room_id 
    FROM userroom
    WHERE user_id = :id
)
LIMIT :limit 
OFFSET :offset;

LIMIT, OFFSET and the userId are dynamically set. If the user is logged in the list doesn't include rooms the user has already joined.

How it looks

sidebar

Critique is welcome and please test it with a bigger data set I just played around with one user. If I should reduce the pull request into 1 or 2 (client/server) commits please let me know.

KenavR commented 8 years ago

Just found a bug with the order of the list on the front end, I will fix it soon. => Fixed

megamattron commented 8 years ago

Thanks! @rickhanlonii do you want to check out the front end on this, and I'll take a look at backend?

rickhanlonii commented 8 years ago

@megamattron yeah I'll check this out 👌

rickhanlonii commented 8 years ago

Hey @KenavR, thanks again for filing. I made a few comments, please review and let me know if you have any questions 👌

megamattron commented 8 years ago

Only one question on the backend @KenavR - you've made an ActiveRoomsService singleton but then don't really have any state in the object, i.e. you load the active rooms from the DB every request. Was the idea that we'd cache those results at some point? Only refresh them every X minutes or something?

Other than that good to go, thanks for the PR!

KenavR commented 8 years ago

@megamattron Actually thinking about it, I have no idea why I made it a singleton, guess I have forgotten how to structure the code without dependency injection. I could change it if you like.

I guess caching would be desirable but with the current query this isn't really possible or wouldn't have that big of an impact, since the result is dependent on the user and in which room he is. If you are fine with listing rooms the user is already in, caching would have a bigger impact - performance wise.

@rickhanlonii I have fixed some of your comments and answered the ones I have problems with - selector/reselect. Maybe you can give me some pointers.

KenavR commented 8 years ago

Ok I figured it out, I moved findLastRank to the selector. Also as mentioned in the other PR, maybe @rickhanlonii wants to take a second look at the way rooms get opened, I don't use the chat-action and just change the url.

KenavR commented 8 years ago

A thought regarding caching, we could cache the top 30 (or something) rooms and filter it based on the user in code. This would reduce the database reads and has all the advantages that come with caching. The cache could be updated sporadically to keep it up to date.

megamattron commented 8 years ago

Agreed, I think this is the way to do it.

On Monday, June 20, 2016, KenavR notifications@github.com wrote:

A thought regarding caching, we could cache the top 30 (or something) rooms and filter it based on the user in code. This would reduce the database reads and has all the advantages that come with caching. The cache could be updated sporadically to keep it up to date.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/larvalabs/breaker/pull/23#issuecomment-227079602, or mute the thread https://github.com/notifications/unsubscribe/AAY8ltbjo71f7KmOxLvaObqPC9evO4fHks5qNk6zgaJpZM4I0R3x .

KenavR commented 8 years ago

Ok I will change the PR on Wednesday.

KenavR commented 8 years ago

Sadly still not finished, I will hopefully finish it tomorrow.

Things to think about

Since the list shouldn't display a room the user has already joined, the joined rooms need to be known to the "load more" endpoint. This information is not cached, therefore I currently make a database query to get it.

I added a couple of new classes, the code in them could be added to some of the existing classes, but I find the code base a lot simpler to understand if the code isn't only separated by architecture (model, service, ...) but also by feature/data type. For example I added a JsonActiveRoomsUtil class instead of adding the code to JsonUtil and I also added an ActiveRoomsRedisUtil class instead of adding the code to the general RedisUtil class. I just noticed that the naming is a little bit inconsistent, but I will fix that in the next update. If you rather keep it all together let me know and I can refactor it or at least keep it in mind for future pull requests.

Missing

KenavR commented 8 years ago

Version 2 seems to be finished, but since I had to change quite a lot, please test it with a good dataset, my test data is very limited.

The collapse function currently resets the list and the data needs to get fetched again from the server. I think that isn't that big of an issue, since the list could change and after a user got through the rooms he won't open the list for quite some time anyway.

@rickhanlonii currently most of the code for the list is inside the sidebar component, let me know if I should extract it into a SidebarActiveRoomListcomponent.

@megamattron please also check if I handle exceptions corretly - https://github.com/KenavR/breaker/blob/bd81172c2e2d056144cff3e80fa0bfb53077acab/app/com/larvalabs/redditchat/util/ActiveRoomsRedisUtil.java#L48-65

megamattron commented 8 years ago

Ok I've been through everything and decided to merge it. I'm going to release and check it out on the main site. I suspect the initial state loading will slow down too much with this new stuff added in there, but I'll take a look at it when it's live and let you know.

megamattron commented 8 years ago

It does indeed seem to be pretty slow, at least for people in a lot of rooms. I'm in a couple hundred rooms and it takes around ~3 seconds to load the initial state now. Used to be down around 500-800ms.

We have a couple options:

  1. I can try to optimize things, or cache stuff, etc.
  2. We don't load the active rooms in the initial state and instead load them async via the endpoint after the page is loaded.

I'm leaning towards the second option, what do you think @KenavR ?

KenavR commented 8 years ago

That's definitely a problem with the amount of rooms you are in, but I already know how to fix it or at least make it a lot better. If that still doesn't improve it enough I would also prefer the second option since that's the solution I suggested at the beginning.

I added a new PR #41