-
-
Notifications
You must be signed in to change notification settings - Fork 879
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
Calculate initial hot_rank and hot_rank_active for posts and comments from other instances #3131
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See my comments in #3125
d694942
to
60c4b6a
Compare
60c4b6a
to
c8aa787
Compare
Trigger changes removed and hot rank calculations added to the respective |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Nutomic is there any other place you can think of where new comments / posts get inserted or updated?
// Calculate initial hot_rank | ||
let conn = &mut get_conn(context.pool()).await?; | ||
|
||
diesel::update(comment_aggregates::table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks correct, but don't run diesel directly here. Add a helper function to either crates/db_schema/src/impls/comment.rs
, (or create a new one at crates/db_schema/src/impls/comment_aggregates.rs
, and call it like:
CommentAggregates::update_hot_ranks(context.pool(), comment_id).await?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like there's already comment_aggregates logic in crates/db_schema/src/aggregates/comment_aggregates.rs
- is there any reason to not add this update_hot_ranks
function there?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see that now, ya that's the place. We should probably move those to impls to be consistent with the others, but we'll handle that later.
crates/apub/Cargo.toml
Outdated
@@ -21,6 +21,7 @@ lemmy_db_views_actor = { workspace = true, features = ["full"] } | |||
lemmy_api_common = { workspace = true, features = ["full"] } | |||
activitypub_federation = { workspace = true } | |||
diesel = { workspace = true } | |||
diesel-async = { workspace = true } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment below, and you'll be able to remove this.
// Calculate initial hot_rank for post | ||
let conn = &mut get_conn(context.pool()).await?; | ||
|
||
diesel::update(post_aggregates::table) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, add this as a helper function to crates/db_schema/src/impls/post.rs
or crates/db_schema/src/impls/post_aggregates.rs
I created the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thx! Don't worry, we'll get this into 0.18.0
.
@@ -191,6 +192,9 @@ impl ActivityHandler for CreateOrUpdateNote { | |||
}; | |||
CommentLike::like(context.pool(), &like_form).await?; | |||
|
|||
// Calculate initial hot_rank | |||
CommentAggregates::update_hot_rank(context.pool(), comment.id).await?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect.
This PR changes how the initial value of
hot_rank
andhot_rank_active
is set for all rows inserted intocomment_aggregates
andpost_aggregates
.Previously, all new rows started at 1728, but with this PR, if an entry is older than 24h, it starts with a rank of 0. If indeed the rank should be higher, then it will correct itself on the next run of the scheduled tasks which update the ranks.
Note that while it would be cool to already calculate the actual hot_rank in these triggers, it's unfortunately not possible as we don't yet have the scores at this point.
Fixes #3125