j3k0 / ganomede-notifications

Long-pull notification service for Ganomede
0 stars 0 forks source link

Online list #12

Closed elmigranto closed 9 years ago

elmigranto commented 9 years ago

Maintains a list of Users who recently had been online.

When the client asks for notifications for user, we add him to the top of this list. Stored as redis' Ordered Set (sorted by -timestamp of request); set is capped at config.options.onlineSize, most recent requests are at the top of the list.

List is publicly available via GET /{prefix}/online.

elmigranto commented 9 years ago

Ready for review and feedback, @j3k0.

j3k0 commented 9 years ago

Hey, all good. But can I ask for some refactoring?

Also, I think you should use LPUSH + LTRIM (both O(1)) instead of ZADD and ZREMOVEBYRANK (O(N) and O(N)+M). No need to sort by timestamp, that'll only sort the list by insertion time.

elmigranto commented 9 years ago

I think you should use LPUSH + LTRIM

The problem with it is duplicate usernames. We'd need to get the whole list every time, remove dupes manually and save the whole list back removing old one resulting in a bunch of O(N) operations.

Also, ZADD is log(N) and ZREMRANGEBYRANK is log(N) + N_removed_elements and we always remove 1 element. Anyway, we only have 20 elements for now, so not much difference here.

Oops, clicked Close&Comment by mistake.

j3k0 commented 9 years ago

good point for zadd. i was wrong, it's better the way you did it.

j3k0 commented 9 years ago

@elmigranto merged, yet i'd like the cleanup if possible... keeping as little online-related code in the notification-api folder

elmigranto commented 9 years ago

Sure, didn't mentioned it in comment, but will work on decoupling this more later. Will also update it to separate redis config.