Keep references to all reblogs of a status on home feed #5419
Conversation
Gargron
force-pushed
the
fix-unreblogs
branch
from
c894f98
to
9d4bc21
Compare
| redis.del(FeedManager.instance.key(:home, account_id)) | ||
| redis.del(FeedManager.instance.key(:home, account_id, 'reblogs')) |
There was a problem hiding this comment.
Oops, missed that somehow. Good call.
| end | ||
| end | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def inactive_users | ||
| User.confirmed.inactive | ||
| @inactive_users ||= User.confirmed.inactive.pluck(:account_id) |
There was a problem hiding this comment.
Style nit that this should maybe become inactive_user_ids, but it's only defined in this tiny class, so whatever.
There was a problem hiding this comment.
Yeah that's uh, not substantial.
| # If the original status or a reblog of it is within | ||
| # REBLOG_FALLOFF statuses from the top, do not re-insert it into | ||
| # the feed | ||
| rank = redis.zrevrank(timeline_key, status.reblog_of_id) | ||
|
|
||
| redis.sadd(reblog_set_key, status.reblog_of_id) unless rank.nil? |
There was a problem hiding this comment.
Does this get us anything? I think it might be best to only put reblogs in the reblogs set: if the original status is deleted, the reblogs should go with it (and we should check for that case in unpush), but there's nothing obvious to be gained by tracking the fact that the original status was in the timeline at some point, is there?
There was a problem hiding this comment.
If the final reblog is deleted, but the original was originally in the feed, we want to re-insert the original - no?
There was a problem hiding this comment.
We don't remove the original when we see the reblog (and in fact, we ignore the reblog), so no, we probably don't need to do anything to track the old status, and should move the other sadd to below the return false.
| # If the original status or a reblog of it is within | ||
| # REBLOG_FALLOFF statuses from the top, do not re-insert it into | ||
| # the feed | ||
| rank = redis.zrevrank(timeline_key, status.reblog_of_id) | ||
|
|
||
| redis.sadd(reblog_set_key, status.reblog_of_id) unless rank.nil? | ||
| redis.sadd(reblog_set_key, status.id) |
There was a problem hiding this comment.
If you're concerned about Redis usage, we can alternatively do this only if another reblog is already within REBLOG_FALLOFF. It requires a bit more logic (to insert the other reblogging status, but only the first time, when creating the set), but doesn't increase Redis usage in the possibly-most-common case of only seeing one reblog.
There was a problem hiding this comment.
Mhhh I don't know what you mean
There was a problem hiding this comment.
If we want to minimize the stuff we add to Redis, we could:
- Not add to reblog_set_key for the first reblog.
- Only add to reblog_set_key for the second-or-later reblog.
(I had thought that we'd have to add the original reblog to reblog_set_key, but I actually don't think we would: if it gets deleted, our first move would be to remove it from reblog_set_key anyway. If we do want to track all of the reblogs for a status that a user could see (which would be nice to expose in the UI, but not what this PR appears to be about), then we'd have to add to that set, but for the limited purposes of this PR, we could get away with not creating the extra set in cases where there was only one user-visible reblog for a given status.)
| @@ -188,7 +194,7 @@ def add_to_feed(timeline_type, account, status) | |||
| # do so if appropriate. | |||
| def remove_from_feed(timeline_type, account, status) | |||
| timeline_key = key(timeline_type, account.id) | |||
| reblog_key = key(timeline_type, account.id, 'reblogs') | |||
| reblog_key = key(timeline_type, account.id, 'reblogs') | |||
|
|
|||
| if status.reblog? | |||
There was a problem hiding this comment.
We should handle the case where status.reblog? == false and unconditionally delete the reblog_set_key, I think? If a status is deleted, we can safely toss all reblogs of it. (I don't actually know what happens right now in the case where we only have a reblog in our feed and the original is deleted, but it might be worth looking into.)
When inserting reblog: Add to set of reblogs of this status on the feed, if original status was present in the feed, add it to that set as well. When removing a reblog: Remove it from that set. Take random remaining item from the set. If one exists, re-insert it into feed, otherwise do not re-insert anything. Fix #4210
Gargron
force-pushed
the
fix-unreblogs
branch
from
9d4bc21
to
13c4f70
Compare
|
@aschmitz Can we merge this and you submit a PR with patches you suggested? I'm.. really tired.. 😩 |
|
Sure.
|
Builds on mastodon#5419, with a few minor optimizations and cleanup of sets after they are no longer needed.
* Clean up reblog-tracking sets from FeedManager Builds on #5419, with a few minor optimizations and cleanup of sets after they are no longer needed. * Update tests, fix multiply-reblogged case Previously, we would have lost the fact that a given status was reblogged if the displayed reblog of it was removed, now we don't. Also added tests to make sure FeedManager#trim cleans up our reblog tracking keys, fixed up FeedCleanupScheduler to use the right loop, and fixed the test for it.
* Keep references to all reblogs of a status on home feed When inserting reblog: Add to set of reblogs of this status on the feed, if original status was present in the feed, add it to that set as well. When removing a reblog: Remove it from that set. Take random remaining item from the set. If one exists, re-insert it into feed, otherwise do not re-insert anything. Fix mastodon#4210 * When original is removed, toss out reblog references
* Clean up reblog-tracking sets from FeedManager Builds on mastodon#5419, with a few minor optimizations and cleanup of sets after they are no longer needed. * Update tests, fix multiply-reblogged case Previously, we would have lost the fact that a given status was reblogged if the displayed reblog of it was removed, now we don't. Also added tests to make sure FeedManager#trim cleans up our reblog tracking keys, fixed up FeedCleanupScheduler to use the right loop, and fixed the test for it.
When inserting reblog: Add to set of reblogs of this status on
the feed, if original status was present in the feed, add it to
that set as well.
When removing a reblog: Remove it from that set. Take random
remaining item from the set. If one exists, re-insert it into feed,
otherwise do not re-insert anything.
Fix #4210