Skip to content

Active support cache - memcached store - consume dalli’s cache_nils option as skip_nil#39763

Merged
rafaelfranca merged 1 commit intorails:masterfrom
ritikesh:dalli_cache_nils
Dec 18, 2020
Merged

Active support cache - memcached store - consume dalli’s cache_nils option as skip_nil#39763
rafaelfranca merged 1 commit intorails:masterfrom
ritikesh:dalli_cache_nils

Conversation

@ritikesh
Copy link
Contributor

@ritikesh ritikesh commented Jun 30, 2020

Summary

Dalli provides an option to cache_nils as a performance optimisation configuration - https://github.com/petergoldstein/dalli/#configuration -> cache_nils.

Rails' MemCacheStore doesn't handle this config very well. If passed, it returns an object of Dalli::Server::NOT_FOUND. It should ideally return nil.

Solutions would be:

  1. return nil instead of Dalli::Server::NOT_FOUND for every deserialize call - this was my first take on the issue - respect dalli’s cache_nil setting #39702. But it felt less intuitive.
  2. if cache_nils is passed as an option to mem_cache_store, convert it to skip_nil.

Before

# config/application.rb
config.cache_store = :mem_cache_store, { 
    servers: %w(memcached:11211),
    namespace: 'namespace',
    compress: true,
    cache_nils: true
}

> Rails.cache.fetch(:z) { nil }
=> #<Dalli::Server::NilObject:0x00007ff30da57028>
> Rails.cache.fetch(:z) { 1 }
=> #<Dalli::Server::NilObject:0x00007ff30da57028>

After

# config/application.rb
config.cache_store = :mem_cache_store, { 
    servers: %w(memcached:11211),
    namespace: 'namespace',
    compress: true,
    cache_nils: true
}

> Rails.cache.fetch(:z) { nil }
=> nil
> Rails.cache.fetch(:z) { 1 }
=> nil

@ritikesh ritikesh changed the title consume dalli’s cache_nils as skip_nil Active support cache - memcached store - consume dalli’s cache_nils option as skip_nil Oct 21, 2020
@ritikesh
Copy link
Contributor Author

ritikesh commented Dec 17, 2020

@rafaelfranca - any thoughts on this?

@rafaelfranca
Copy link
Member

rafaelfranca commented Dec 17, 2020

This makes sense. Can you add a CHANGELOG entry and squash the commits in one?

@ritikesh
Copy link
Contributor Author

ritikesh commented Dec 18, 2020

@rafaelfranca - done, thanks.

@rafaelfranca rafaelfranca merged commit 000081d into rails:master Dec 18, 2020
@ritikesh ritikesh deleted the dalli_cache_nils branch December 18, 2020 21:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

activesupport

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@ritikesh @rafaelfranca