Skip to content

Max size option - #15

Open
Qqwy wants to merge 6 commits into
bsm:mainfrom
Qqwy:max_size
Open

Max size option#15
Qqwy wants to merge 6 commits into
bsm:mainfrom
Qqwy:max_size

Conversation

@Qqwy

@Qqwy Qqwy commented Dec 20, 2022

Copy link
Copy Markdown

Implements a max_size option that ensures that the cache database table will not exceed a given amount of records.

This PR depends on PR #14 .


# 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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

# 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.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

typo/correct wording: "this is the maximum number of entries..."

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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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:

  1. you could make cleanup return an integer (which I think it already does) to indicate the number of records removed, that would potentially save you a query
  2. always calling max_size_exceeded? twice is not necessary anyway, e.g. if the first attempt already returns false

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants