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

Fix RedisCacheStore#write_multi to properly update the local store. #43042

Closed

Conversation

casperisfine
Copy link
Contributor

RedisCacheStore specialises write_multi to use mset when possible, but it didn't update the local cache accordingly.

Unless I'm mistaken, I think the bug was present in the initial version #31134

cc @jeremy

`RedisCacheStore` specialises `write_multi` to use `mset` when possible,
but it didn't update the local cache accordingly.
end

if local_cache
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we can use local_cache directly here, see #38630 (comment).

How about overriding write_multi_entries in the LocalCache module to clear the entries' keys?

def read_multi_entries(keys, **options)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can, but we'll do the work multiple times in most cases as except for this one special cache for redis, write_multi_entries is:

        def write_multi_entries(hash, **options)
          hash.each do |key, entry|
            write_entry key, entry, **options
          end
        end

But given the restrictions on that specialized write_multi (only used if no expiry), I wonder if we wouldn't be better to just remove it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That being said, "the work" is a simple Hash#[]= per key. So might not matter much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @rafaelfranca what do you think?

@rails-bot
Copy link

rails-bot bot commented Nov 16, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Nov 16, 2021
@rails-bot
Copy link

rails-bot bot commented Feb 14, 2022

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.
Thank you for your contributions.

@rails-bot rails-bot bot added the stale label Feb 14, 2022
@rails-bot rails-bot bot closed this Feb 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants