Max size option #15
Max size option #15
Conversation
| @@ -20,8 +20,15 @@ def self.supports_cache_versioning? | |||
|
|
|||
| # param [Hash] options options | |||
| # option options [Class] :model model class. Default: ActiveSupport::Cache::DatabaseStore::Model | |||
| # option options [Boolean] :auto_cleanup When true, runs {#cleanup} after every {#write_entry} and {#delete_entry}. Default: false | |||
There was a problem hiding this comment.
Sorry, thank you very much for the contribution, but could you please submit this as an independent PR? It's so much easier to review and merge them individually, as separate features.
| @@ -20,8 +20,15 @@ def self.supports_cache_versioning? | |||
|
|
|||
| # param [Hash] options options | |||
| # option options [Class] :model model class. Default: ActiveSupport::Cache::DatabaseStore::Model | |||
| # option options [Boolean] :auto_cleanup When true, runs {#cleanup} after every {#write_entry} and {#delete_entry}. Default: false | |||
| # option options [Integer, nil] :max_size When set (to a positive integer), | |||
| # this is the maximum amount of entries that is allowed in the cache. | |||
There was a problem hiding this comment.
typo/correct wording: "this is the maximum number of entries..."
| @@ -74,10 +81,23 @@ def write_entry(key, entry, _options = nil) | |||
| record = @model.where(key: key).first_or_initialize | |||
| expires_at = Time.zone.at(entry.expires_at) if entry.expires_at | |||
| record.update! value: Marshal.dump(entry.value), version: entry.version.presence, expires_at: expires_at | |||
| ensure | |||
| cleanup if @auto_cleanup || max_size_exceeded? | |||
| delete_oldest_entry if max_size_exceeded? # <- Only happens when running cleanup was not enough | |||
There was a problem hiding this comment.
I can see this being a useful thing, but I am worried about the number of SQL queries that it requires (up to 6!). In the same vein as with #14 I wonder if this is better implemented as a timed option. Instead of running up-to 6 queries on every write, ensure there is at least e.g. 1 minute between each check, i.e. make this an "approximate" rather than an absolute thing.
Furthermore, you could optimise here:
- you could make
cleanupreturn an integer (which I think it already does) to indicate the number of records removed, that would potentially save you a query - always calling
max_size_exceeded?twice is not necessary anyway, e.g. if the first attempt already returns false
Implements a
max_sizeoption that ensures that the cache database table will not exceed a given amount of records.This PR depends on PR #14 .