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

Bust cache on article destroy instead of resave articles #1621 #1801

Merged
merged 2 commits into from Feb 24, 2019

Conversation

lightalloy
Copy link
Contributor

@lightalloy lightalloy commented Feb 14, 2019

What type of PR is this? (check all applicable)

  • Refactor

Description

In the current implementation, when an article is destroyed, all the "sibling" articles of the (destroyed) article organization are resaved. Resaving triggers several callbacks, which create a lot of delayed_jobs. That takes plenty of time and resources. There's no need to resave articles, it's enough to bust their cache.

What happens on article destroy after the refactoring:

  • instead of resaving organization articles, their cache is busted
  • user articles cache is busted asynchronously

Related Tickets & Documents

#1621
Not directly related to the ticket (not fixing errors), but contains refactoring for the related code.

@lightalloy lightalloy changed the title [WIP] Bust cache on article destroy instead of resave articles #1621 Bust cache on article destroy instead of resave articles #1621 Feb 14, 2019
@pr-triage pr-triage bot added the PR: unreviewed bot applied label for PR's with no review label Feb 14, 2019
article_ids.concat organization.article_ids
end
# perform busting cache in chunks in case there're a lot of articles
(article_ids.uniq.sort - [id]).each_slice(10) do |ids|
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting here is just to be able to test, that the job is enqueued with the correct arguments (spec/models/article_destroy_spec.rb:24). The order of article_ids is not important. Sets are not allowed as job arguments.
If anyone has a better idea to remove sorting and still test, please, share.

Copy link
Contributor

@Zhao-Andy Zhao-Andy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good! After looking through RSpec's API docs, I couldn't find a better way to avoid sorting the array. I'm guessing it's not a huge performance issue, so it should be okay to sort the array.

@pr-triage pr-triage bot added PR: reviewed-approved bot applied label for PR's where reviewer approves changes and removed PR: unreviewed bot applied label for PR's with no review labels Feb 21, 2019
@benhalpern benhalpern merged commit e45c61a into forem:master Feb 24, 2019
@pr-triage pr-triage bot added PR: merged bot applied label for PR's that are merged and removed PR: reviewed-approved bot applied label for PR's where reviewer approves changes labels Feb 24, 2019
@lightalloy lightalloy deleted the optimize-article-destroy branch March 9, 2020 11:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: merged bot applied label for PR's that are merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants