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

Max size option #15

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions lib/active_support/cache/database_store.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
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 [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
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..."

# Going over will first run {#cleanup} to delete any expired entries.
# If this is not enough, the oldest entry in the cache (based on `created_at` time) will also be deleted. Default: nil
def initialize(options = nil)
@model = (options || {}).delete(:model) || Model
@auto_cleanup = (options || {}).delete(:auto_cleanup) || false
@max_size = (options || {}).delete(:max_size) || nil
super(options)
end

Expand Down Expand Up @@ -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
Copy link
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

end

def max_size_exceeded?
@max_size && @model.count > @max_size
end

def delete_oldest_entry
@model.order(created_at: :asc).limit(1).destroy_all
end

def delete_entry(key, _options = nil)
@model.where(key: key).destroy_all
ensure
cleanup if @auto_cleanup
end

def read_multi_entries(names, options)
Expand Down
128 changes: 128 additions & 0 deletions spec/active_support/cache/database_store_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -326,4 +326,132 @@ def obj.cache_key_with_version
expect(subject.read('foo', version: '1')).to eq('bar')
end
end

describe "auto_cleanup option" do
describe "when true" do
subject do
described_class.new auto_cleanup: true
end

it "cleans up expired entries automatically on write" do
time = Time.now
expect(subject.count(all: true)).to eq(0)

subject.write('one', 'foo', expires_in: 1)
subject.write('two', 'bar', expires_in: 1)
subject.write('three', 'baz', expires_in: 1)

expect(subject.count(all: true)).to eq(3)

allow(Time).to receive(:now).and_return(time + 11)

expect(subject.count(all: true)).to eq(3)

subject.write('four', 'qux', expires_in: 1)

expect(subject.count(all: true)).to eq(1)
expect(subject.read('four')).to eq('qux')
end

it "cleans up expired entries automatically on delete" do
time = Time.now
expect(subject.count(all: true)).to eq(0)

subject.write('one', 'foo', expires_in: 1)
subject.write('two', 'bar', expires_in: 1)
subject.write('three', 'baz', expires_in: 1)

expect(subject.count(all: true)).to eq(3)

allow(Time).to receive(:now).and_return(time + 11)

expect(subject.count(all: true)).to eq(3)

subject.delete('three')

expect(subject.count(all: true)).to eq(0)
end
end

describe "when false or not set" do
subject do
described_class.new auto_cleanup: false
end

it "no cleanup happens on write" do
time = Time.now
expect(subject.count(all: true)).to eq(0)

subject.write('one', 'foo', expires_in: 1)
subject.write('two', 'bar', expires_in: 1)
subject.write('three', 'baz', expires_in: 1)

expect(subject.count(all: true)).to eq(3)

allow(Time).to receive(:now).and_return(time + 11)

expect(subject.count(all: true)).to eq(3)

subject.write('four', 'qux', expires_in: 1)

expect(subject.count(all: true)).to eq(4)
expect(subject.read('four')).to eq('qux')
end
end

it "no cleanup happens on delete" do
time = Time.now
expect(subject.count(all: true)).to eq(0)

subject.write('one', 'foo', expires_in: 1)
subject.write('two', 'bar', expires_in: 1)
subject.write('three', 'baz', expires_in: 1)

expect(subject.count(all: true)).to eq(3)

allow(Time).to receive(:now).and_return(time + 11)

expect(subject.count(all: true)).to eq(3)

subject.delete('three')

expect(subject.count(all: true)).to eq(2)
end
end

describe "max_size option" do
subject do
described_class.new max_size: 3
end

it "when set, deletes oldest entry when exceeding" do
subject.write('one', 'foo')
subject.write('two', 'bar')
subject.write('three', 'baz')
subject.write('four', 'qux')

expect(subject.count(all: true)).to eq(3)
expect(subject.read('four')).to eq('qux')
expect(subject.read('three')).to eq('baz')
expect(subject.read('two')).to eq('bar')
expect(subject.read('one')).to eq(nil)
end

it "when set, first calls cleanup when exceeding" do
time = Time.now
expect(subject.count(all: true)).to eq(0)

subject.write('one', 'foo', expires_in: 1)
subject.write('two', 'bar', expires_in: 1)
subject.write('three', 'baz', expires_in: 100)

expect(subject.count(all: true)).to eq(3)

allow(Time).to receive(:now).and_return(time + 2)

subject.write('four', 'baz', expires_in: 1)

expect(subject.count(all: true)).to eq(2)
end
end
end