-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
Use node views cache #5338
Use node views cache #5338
Conversation
This should improve performance of various page loads, as it avoids running COUNT() queries on the impressions table, and instead uses the views value, which the impressions gem keeps up-to-date.
Generated by 🚫 Danger |
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.
Hi, changes looks good to me. Thanks for your work.
@gauravano can you please review this and merge this?
@@ -81,7 +81,7 @@ def teardown | |||
assert_equal '0.0.0.0', Impression.last.ip_address | |||
Impression.last.update_attribute('ip_address', '0.0.0.1') | |||
|
|||
assert_difference 'note.totalviews', 1 do | |||
assert_difference 'note.reload.totalviews', 1 do |
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.
Oh interesting. Can we add a bit of documentation about this usage? I'm not familiar with it! Thanks!
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 your note above:
Notice that I update the tests to .reload the node before reading values,
because ActiveRecord caches those, which is why tests fail without it.
Does it cache it just within the context of the test? Any other situations where we'd need to flush the cache? Thank you!!!
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.
From my experience with Rails, this is a tests-specific hack. The cache is on request level, so it shouldn't affect any production paths.
In this specific test, we're reading the note, then changing the impressions, then trying to read a value from the note. Since the note is cached (i.e. already read), we have to force it to reload from DB, so that we get the most recent value of totalviews
.
All right -- this is great then! Merging now!!! 🎉 Thanks for your patience!!! |
* Restore using node.views for unique impressions count This should improve performance of various page loads, as it avoids running COUNT() queries on the impressions table, and instead uses the views value, which the impressions gem keeps up-to-date. * Added a migration which updates node.views to the count of unique visits
* Restore using node.views for unique impressions count This should improve performance of various page loads, as it avoids running COUNT() queries on the impressions table, and instead uses the views value, which the impressions gem keeps up-to-date. * Added a migration which updates node.views to the count of unique visits
Fixes #1196
This should improve performance of various page loads, as it avoids
running COUNT() queries on the impressions table, and instead uses the
views value, which the impressions gem keeps up-to-date.
Notice that I update the tests to
.reload
the node before reading values,because ActiveRecord caches those, which is why tests fail without it.
Make sure these boxes are checked before your pull request (PR) is ready to be reviewed and merged. Thanks!
rake test
@publiclab/reviewers
for help, in a comment belowIf tests do fail, click on the red
X
to learn why by reading the logs.Please be sure you've reviewed our contribution guidelines at https://publiclab.org/contributing-to-public-lab-software
Thanks!