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

Notifications.prune taking too long #999

Closed
barisusakli opened this Issue Feb 11, 2014 · 8 comments

Comments

Projects
None yet
4 participants
@barisusakli
Member

barisusakli commented Feb 11, 2014

This function needs rethinking, right now takes up to a minute to process causing the forum to become unresponsive.

Instead of running it on a cron for all users it can be run on a user basis when they receive a notification.

@barisusakli barisusakli added this to the 0.3.2 milestone Feb 11, 2014

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam Feb 11, 2014

Member

Running it on a per-user basis would be fine, unless a user does not log in...

Notifications cron should only be run once a day anyway... when did it run for you?

Member

julianlam commented Feb 11, 2014

Running it on a per-user basis would be fine, unless a user does not log in...

Notifications cron should only be run once a day anyway... when did it run for you?

@barisusakli

This comment has been minimized.

Show comment
Hide comment
@barisusakli

barisusakli Feb 11, 2014

Member

Hmm I think its running 7pm est. Even with 650 users its taking 52 seconds, so it will be a huge problem on bigger forums.

Not really sure how the notification pruning works, I think it doesnt prune anything if they are not read? So if a user never logs in the notifications just keep increasing forever?

Member

barisusakli commented Feb 11, 2014

Hmm I think its running 7pm est. Even with 650 users its taking 52 seconds, so it will be a huge problem on bigger forums.

Not really sure how the notification pruning works, I think it doesnt prune anything if they are not read? So if a user never logs in the notifications just keep increasing forever?

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam Feb 11, 2014

Member

Yes. I'll be the first to admit that notifications need an overhaul.

Notification pruning works thusly:

  1. For every notification in the notifications set, check its expiry date
  2. Also retrieve a copy of every user's inbox
  3. If a notif has expired, and is not present in any user inboxi, delete it.

Steps 2 and 3 are obviously the O(derp) part, because the more users there are, the longer it will take. Step 3 is, I believe, running .every to ensure that the nid is not present in inboxes, but the inboxes array gets increasingly larger.

Instead, I think we should simpify the system:

  • Broad delete all notifications older than x days (admin definable). Do not bother deleting it from user inboxes (uid:{uid}:notifications:unread)
  • If a user requests a notification that has been deleted (nid-not-found), delete that reference from their inbox set.
Member

julianlam commented Feb 11, 2014

Yes. I'll be the first to admit that notifications need an overhaul.

Notification pruning works thusly:

  1. For every notification in the notifications set, check its expiry date
  2. Also retrieve a copy of every user's inbox
  3. If a notif has expired, and is not present in any user inboxi, delete it.

Steps 2 and 3 are obviously the O(derp) part, because the more users there are, the longer it will take. Step 3 is, I believe, running .every to ensure that the nid is not present in inboxes, but the inboxes array gets increasingly larger.

Instead, I think we should simpify the system:

  • Broad delete all notifications older than x days (admin definable). Do not bother deleting it from user inboxes (uid:{uid}:notifications:unread)
  • If a user requests a notification that has been deleted (nid-not-found), delete that reference from their inbox set.
@psychobunny

This comment has been minimized.

Show comment
Hide comment
@psychobunny

psychobunny Feb 11, 2014

Member

Something I forgot to reply about much earlier:

Running it on a per-user basis would be fine, unless a user does not log in...

Not the case if so:

Instead of running it on a cron for all users it can be run on a user basis when they receive a notification.

Member

psychobunny commented Feb 11, 2014

Something I forgot to reply about much earlier:

Running it on a per-user basis would be fine, unless a user does not log in...

Not the case if so:

Instead of running it on a cron for all users it can be run on a user basis when they receive a notification.

@julianlam julianlam modified the milestones: 0.4.0, 0.3.2 Feb 14, 2014

@barisusakli barisusakli modified the milestones: 0.3.2, 0.4.0 Feb 14, 2014

@barisusakli barisusakli self-assigned this Feb 14, 2014

@julianlam julianlam modified the milestone: 0.3.2 Feb 14, 2014

@ThisIsMissEm

This comment has been minimized.

Show comment
Hide comment
@ThisIsMissEm

ThisIsMissEm Feb 14, 2014

Contributor

The simplest possible solution would be to use a Child Process to do this work instead, hence not blocking the main web server thread.

Contributor

ThisIsMissEm commented Feb 14, 2014

The simplest possible solution would be to use a Child Process to do this work instead, hence not blocking the main web server thread.

@ThisIsMissEm

This comment has been minimized.

Show comment
Hide comment
@ThisIsMissEm

ThisIsMissEm Feb 14, 2014

Contributor

That could be done with https://www.npmjs.org/package/cronos potentially.

Contributor

ThisIsMissEm commented Feb 14, 2014

That could be done with https://www.npmjs.org/package/cronos potentially.

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam Feb 14, 2014

Member

I believe the second task (in the ones I listed above) is already done, so I'll go ahead and just have prune delete the expired notification objects after 7 days.

Can extend this to be an admin definable value, but is not necessary at the moment.

Member

julianlam commented Feb 14, 2014

I believe the second task (in the ones I listed above) is already done, so I'll go ahead and just have prune delete the expired notification objects after 7 days.

Can extend this to be an admin definable value, but is not necessary at the moment.

@julianlam

This comment has been minimized.

Show comment
Hide comment
@julianlam

julianlam Feb 14, 2014

Member

Hahaha, I like how the "status" for that module is "Alpha as f*ck". NodeBB already uses cron as a dependency, so no need to change, I think...

Member

julianlam commented Feb 14, 2014

Hahaha, I like how the "status" for that module is "Alpha as f*ck". NodeBB already uses cron as a dependency, so no need to change, I think...

julianlam added a commit that referenced this issue Feb 14, 2014

julianlam added a commit that referenced this issue Feb 14, 2014

@julianlam julianlam closed this Feb 14, 2014

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