Skip to content

Turn on active record cache_versioning#1149

Merged
rsanheim merged 3 commits intomasterfrom
turn-on-cache-key-versions
Jul 20, 2020
Merged

Turn on active record cache_versioning#1149
rsanheim merged 3 commits intomasterfrom
turn-on-cache-key-versions

Conversation

@rsanheim
Copy link
Contributor

This keeps the version seperate from Rails standard cache_key, which
allows for better recycling of cached entries as things get updated
freqently. This shouldn't impact our app right now, as we are only
using our own manual cache keys for analytics.

For more details on how recyclable cache keys work, see:

This keeps the version seperate from Rails standard cache_key, which
allows for better recycling of cached entries as things get updated
freqently.  This shouldn't impact our app right now, as we are only
using our own manual cache keys for analytics.

For more details on how recyclable cache keys work, see:

* rails/rails#29092
* https://dzone.com/articles/cache-invalidation-complexity-rails-52-and-dalli-c
@rsanheim rsanheim requested a review from a team July 10, 2020 18:07
@harimohanraj89 harimohanraj89 temporarily deployed to simple-review-pr-1149 July 10, 2020 18:09 Inactive
Copy link
Contributor

@vkrmis vkrmis left a comment

Choose a reason for hiding this comment

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

Are we doing this in preparation for Dashboard work?
Also, can we turn this on seamlessly or do we need to do some prep work before we can do this safely? (eg: flush existing cache)

@rsanheim
Copy link
Contributor Author

Are we doing this in preparation for Dashboard work?

Yup.

Also, can we turn this on seamlessly or do we need to do some prep work before we can do this safely? (eg: flush existing cache)

It is seamless, none of our existing cache usage will be impacted by this. All our existing cache usage uses our own manually constructed keys:

[~/src/simpledotorg/simple-server (master)]> ack cache app
app/exporters/patients_with_history_exporter.rb
163:      @medications = dates.each_with_object({}) { |date, cache|
164:        cache[date] = date ? patient.prescribed_drugs(date: date) : PrescriptionDrug.none

app/presenters/user_analytics_presenter.rb
99:      Rails.cache.fetch(statistics_cache_key, expires_in: EXPIRE_STATISTICS_CACHE_IN) {
382:  def statistics_cache_key

app/controllers/analytics_controller.rb
46:  def set_analytics_cache(key)
47:    Rails.cache.fetch(key, expires_in: ENV.fetch("ANALYTICS_DASHBOARD_CACHE_TTL")) { yield }
50:  def analytics_cache_key_cohort(period)
51:    "#{analytics_cache_key}/cohort/#{period}"
54:  def analytics_cache_key_dashboard(time_period)
55:    key = "#{analytics_cache_key}/dashboard/#{time_period}"

app/controllers/analytics/districts_controller.rb
90:      set_analytics_cache(analytics_cache_key_cohort(period)) {
97:      set_analytics_cache(analytics_cache_key_dashboard(period)) {
121:  def analytics_cache_key

app/controllers/analytics/facilities_controller.rb
87:      set_analytics_cache(analytics_cache_key_cohort(period)) {
94:      set_analytics_cache(analytics_cache_key_dashboard(period)) {
101:  def analytics_cache_key

@rsanheim rsanheim temporarily deployed to simple-review-pr-1149 July 13, 2020 17:51 Inactive
@kitallis
Copy link
Contributor

kitallis commented Jul 14, 2020

If I understand cache_version right, it's basically to avoid key evictions since redis typically LRUs off keys constantly and in a high-traffic, constant version changing keys scenario, you'd end up getting a lot of unnecessary evictions for keys that don't even need to be evicted. This new change patches up the cache store, sticks a version as a part of serialized metadata and upserts it into a base key to avoid unnecessary evictions?

This also sort of effectively changes the definition of a "cache miss" from a redis PoV, but from a rails perspective an unmatched version fetch would be a miss if I'm getting this right.

@rsanheim
Copy link
Contributor Author

If I understand cache_version right, it's basically to avoid key evictions since redis typically LRUs off keys constantly and in a high-traffic, constant version changing keys scenario, you'd end up getting a lot of unnecessary evictions for keys that don't even need to be evicted. This new change patches up the cache store, sticks a version as a part of serialized metadata and upserts it into a base key to avoid unnecessary evictions?

Yup, it essentially separates out the cache version from the key. So your entries are stored under the cache_key only, and then the version is inside the cached entry itself...so the version can be compared and old entries can be removed from the cache if there is a newer entry on a fetch call. So you don't have to rely on Redis' built-in eviction for versioned entries that are changed frequently, which helps with memory usage.

@rsanheim rsanheim merged commit 1c0a08c into master Jul 20, 2020
@rsanheim rsanheim deleted the turn-on-cache-key-versions branch July 20, 2020 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

4 participants