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

Add `delete_multi` method to cache #36927

Merged
merged 1 commit into from Aug 26, 2019

Conversation

@peterzhu2118
Copy link
Contributor

commented Aug 13, 2019

Summary

Adds ActiveSupport::Cache::Store#delete_multi to delete an array of keys from the cache.

For most implementations, this just calls ActiveSupport::Cache::Store#delete n times, but Redis supports deleting an array of keys. This can improve performance because it turns what is previously n delete calls into a single call.

Memcached does not support multi delete.

Copy link
Member

left a comment

Nice feature! Observe the interaction between read_multi (public API implemented once in base class) and read_multi_entries (private implementation; default in base class, overridden in subclasses). You can follow this pattern with delete_multi (public API) and delete_multi_entries (private implementation) as well.

@peterzhu2118 peterzhu2118 force-pushed the peterzhu2118:cache-delete-multi branch 4 times, most recently from e0bcb4e to a862d59 Aug 15, 2019
@peterzhu2118

This comment has been minimized.

Copy link
Contributor Author

commented Aug 15, 2019

Hey @jeremy, thanks for the review! I've updated the implementation with your suggestions, please take another look!

@peterzhu2118 peterzhu2118 force-pushed the peterzhu2118:cache-delete-multi branch from a862d59 to a78958d Aug 15, 2019
@peterzhu2118

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

Hey @jeremy, could I get another review please?

@@ -477,6 +477,18 @@ def delete(name, options = nil)
end
end

# Deletes multiple entries in the cache. Returns the number of entries deleted.

This comment has been minimized.

Copy link
@jeremy

jeremy Aug 20, 2019

Member

Should we introduce this requirement? Redis naturally returns the number of entries deleted, but it may be costly or impractical for other cache stores.

This comment has been minimized.

Copy link
@peterzhu2118

peterzhu2118 Aug 20, 2019

Author Contributor

The regular delete_entry returns a Boolean on whether it successfully deleted an entry or not, so I don't see why this would be more costly since for most cache stores it just calls delete_entry n times. Is there a particular cache store you think could get a performance bump if this requirement was dropped?

This comment has been minimized.

Copy link
@jeremy

jeremy Aug 20, 2019

Member

We are introducing a new API. Do we care about its return value, or should we leave it undefined?

For example, consider a third-party cache store that supports batch deletes but doesn't report the number of actual deletions. Or a cache store that issues deletions asynchronously. Should these implementations attempt to return a count of deleted entries?

This comment has been minimized.

Copy link
@peterzhu2118

peterzhu2118 Aug 20, 2019

Author Contributor

Thanks for the explanation! I've updated it to keep the current logic but removed the return value from the documentation.

@peterzhu2118 peterzhu2118 force-pushed the peterzhu2118:cache-delete-multi branch from a78958d to ff611b4 Aug 20, 2019
@jeremy jeremy merged commit 59e746d into rails:master Aug 26, 2019
2 checks passed
2 checks passed
buildkite/rails Build #63206 passed (10 minutes, 19 seconds)
Details
codeclimate All good!
Details
@jewilmeer

This comment has been minimized.

Copy link

commented Sep 20, 2019

Awesome! We used a custom implementation and can now switch back to the default again! Thanks @peterzhu2118

@cache.write("foo", "bar")
assert @cache.exist?("foo")
@cache.write("hello", "world")
assert @cache.exist?("foo")

This comment has been minimized.

Copy link
@n-rodriguez

n-rodriguez Sep 30, 2019

it it the good assertion here?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.