Skip to content
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

PERF: Cache ranks for featured badges, to simplify user serialization #8698

Merged
merged 7 commits into from Jan 14, 2020

Conversation

@davidtaylorhq
Copy link
Member

davidtaylorhq commented Jan 10, 2020

This introduces a new featured_rank column on the user_badges table. This is kept up-to-date automatically, and makes it much easier to load badges for user profiles and user cards.

@discoursebot

This comment has been minimized.

Copy link

discoursebot commented Jan 10, 2020

You've signed the CLA, davidtaylorhq. Thank you! This pull request is ready for review.

Copy link
Member

eviltrout left a comment

It does seem like updating the featured ranks of all badges could be quite costly. Can you confirm that you tested the update_featured_ranks! method on a large dataset to see how it performs?

app/models/user_badge.rb Outdated Show resolved Hide resolved
app/models/user_badge.rb Outdated Show resolved Hide resolved
@@ -22,6 +22,7 @@ def execute(args)
end

BadgeGranter.revoke_ungranted_titles!
UserBadge.ensure_consistency! # Badge granter sometimes uses raw SQL, so hooks do not run. Clean up data

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Jan 13, 2020

Member

This worries me a bit, what is the cost of the new big query on one of our larger sites?

def change
add_column :user_badges, :featured_rank, :integer, null: true

execute <<~SQL

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Jan 13, 2020

Member

will this lock up the table in a transaction? do we need to be careful here and run it outside of a transaction?

This comment has been minimized.

Copy link
@davidtaylorhq

davidtaylorhq Jan 13, 2020

Author Member

During the transaction, it holds a RowExclusiveLock over the UserBadges table. That means existing rows in the UserBadge table cannot be updated by any other process during this time.

I will add an extra where clause so that it doesn't hold locks over rows that already have the correct rank. (although obviously that will not help for the initial migration)

The most common action is to create new user_badge rows, and that seems unaffected by the RowExclusiveLock. On the largest forums, I think we are looking at < 5 seconds of locking for this migration. Do you think that's ok?

This comment has been minimized.

Copy link
@SamSaffron

SamSaffron Jan 14, 2020

Member

5 seconds on a large db sounds fine to me. 5 minutes and stuff can get a bit tricky.

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Jan 13, 2020

Overall I really like this change, we just have to be super careful with upgrade and need to make sure we are not introducing extreme costs to our daily schedule.

@davidtaylorhq

This comment has been minimized.

Copy link
Member Author

davidtaylorhq commented Jan 13, 2020

Yes, I did a few tests to make sure it's not too expensive. Running this as a SELECT, rather than UPDATE, I tested this on a few sites.

On meta with 41k users, and 94k user_badges, it took 0.6 seconds.
On a site with 4.2 million users, and 590k user_badges, it took 2.3 seconds

@deepcode-ci-bot

This comment has been minimized.

Copy link

deepcode-ci-bot bot commented Jan 13, 2020

DeepCode's analysis on #751648 found:

  • 0 critical issues. ⚠️ 0 warnings and 0 minor issues. ✔️ 0 issues were fixed.

💬 This comment has been generated by the DeepCode bot, installed by the owner of the repository. The DeepCode bot protects your repository by detecting and commenting on security vulnerabilities or other critical issues.


☺️ If you want to provide feedback on our bot, here is how to contact us.

@SamSaffron

This comment has been minimized.

Copy link
Member

SamSaffron commented Jan 14, 2020

This is looking good, feel free to merge

Copy link
Member

ZogStriP left a comment

Once you get @eviltrout's comments in, feel free to merge 👍

Co-Authored-By: Robin Ward <robin.ward@gmail.com>
@davidtaylorhq davidtaylorhq merged commit cff6e94 into discourse:master Jan 14, 2020
5 of 6 checks passed
5 of 6 checks passed
PLUGINS-BACKEND PLUGINS-BACKEND
Details
CORE-BACKEND
Details
PLUGINS-FRONTEND
Details
CORE-FRONTEND
Details
PLUGINS-LINT
Details
CORE-LINT
Details
@davidtaylorhq davidtaylorhq deleted the davidtaylorhq:featured_badge_perf branch Jan 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
5 participants
You can’t perform that action at this time.