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

Refactor DalliStore to better support subclassing #417

Merged
merged 1 commit into from Jan 4, 2014
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
71 changes: 43 additions & 28 deletions lib/active_support/cache/dalli_store.rb
Expand Up @@ -38,7 +38,6 @@ def initialize(*addresses)
options = addresses.extract_options!
@options = options.dup
@options[:compress] ||= @options[:compression]
@raise_errors = !!@options[:raise_errors]
servers = if addresses.empty?
nil # use the default from Dalli::Client
else
Expand All @@ -56,6 +55,10 @@ def dalli
@data
end

def with_connection
yield @data
end

def fetch(name, options=nil)
options ||= {}
name = expanded_key name
Expand Down Expand Up @@ -103,7 +106,10 @@ def write(name, value, options=nil)
name = expanded_key name

instrument(:write, name, options) do |payload|
write_entry(name, value, options)
with_connection do |connection|
options = options.merge(:connection => connection)
write_entry(name, value, options)
end
end
end

Expand Down Expand Up @@ -139,7 +145,8 @@ def read_multi(*names)
end
end

results.merge!(@data.get_multi(mapping.keys - results.keys))
data = with_connection { |c| c.get_multi(mapping.keys - results.keys) }
results.merge!(data)
results.inject({}) do |memo, (inner, _)|
entry = results[inner]
# NB Backwards data compatibility, to be removed at some point
Expand All @@ -160,18 +167,21 @@ def fetch_multi(*names)
mapping = names.inject({}) { |memo, name| memo[expanded_key(name)] = name; memo }

instrument(:fetch_multi, names) do
results = @data.get_multi(mapping.keys)

@data.multi do
mapping.inject({}) do |memo, (expanded, name)|
memo[name] = results[expanded]
if memo[name].nil?
value = yield(name)
memo[name] = value
write_entry(expanded, value, options)
end
with_connection do |connection|
results = connection.get_multi(mapping.keys)

connection.multi do
mapping.inject({}) do |memo, (expanded, name)|
memo[name] = results[expanded]
if memo[name].nil?
value = yield(name)
memo[name] = value
options = options.merge(:connection => connection)
write_entry(expanded, value, options)
Copy link
Collaborator

Choose a reason for hiding this comment

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

write_entry calls with_connection internally, which is messy. I'd change the private methods to accept a client connection to use.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Adding one additional argument to write_entry doesn't work because it needs to have the same number of arguments as ActiveSupport::Cache::Strategy::LocalCache#write_entry. We could pass it the connection in the options hash. Thoughts?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a hack but probably workable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can update the pull request so you can take a better look!

end

memo
memo
end
end
end
end
Expand All @@ -188,11 +198,11 @@ def increment(name, amount = 1, options=nil)
initial = options.has_key?(:initial) ? options[:initial] : amount
expires_in = options[:expires_in]
instrument(:increment, name, :amount => amount) do
@data.incr(name, amount, expires_in, initial)
with_connection { |c| c.incr(name, amount, expires_in, initial) }
end
rescue Dalli::DalliError => e
logger.error("DalliError: #{e.message}") if logger
raise if @raise_errors
raise if raise_errors?
nil
end

Expand All @@ -207,23 +217,23 @@ def decrement(name, amount = 1, options=nil)
initial = options.has_key?(:initial) ? options[:initial] : 0
expires_in = options[:expires_in]
instrument(:decrement, name, :amount => amount) do
@data.decr(name, amount, expires_in, initial)
with_connection { |c| c.decr(name, amount, expires_in, initial) }
end
rescue Dalli::DalliError => e
logger.error("DalliError: #{e.message}") if logger
raise if @raise_errors
raise if raise_errors?
nil
end

# Clear the entire cache on all memcached servers. This method should
# be used with care when using a shared cache.
def clear(options=nil)
instrument(:clear, 'flushing all keys') do
@data.flush_all
with_connection { |c| c.flush_all }
end
rescue Dalli::DalliError => e
logger.error("DalliError: #{e.message}") if logger
raise if @raise_errors
raise if raise_errors?
nil
end

Expand All @@ -233,11 +243,11 @@ def cleanup(options=nil)

# Get the statistics from the memcached servers.
def stats
@data.stats
with_connection { |c| c.stats }
end

def reset
@data.reset
with_connection { |c| c.reset }
end

def logger
Expand All @@ -252,12 +262,12 @@ def logger=(new_logger)

# Read an entry from the cache.
def read_entry(key, options) # :nodoc:
entry = @data.get(key, options)
entry = with_connection { |c| c.get(key, options) }
# NB Backwards data compatibility, to be removed at some point
entry.is_a?(ActiveSupport::Cache::Entry) ? entry.value : entry
rescue Dalli::DalliError => e
logger.error("DalliError: #{e.message}") if logger
raise if @raise_errors
raise if raise_errors?
nil
end

Expand All @@ -267,19 +277,20 @@ def write_entry(key, value, options) # :nodoc:
cleanup if options[:unless_exist]
method = options[:unless_exist] ? :add : :set
expires_in = options[:expires_in]
@data.send(method, key, value, expires_in, options)
connection = options.delete(:connection)
connection.send(method, key, value, expires_in, options)
rescue Dalli::DalliError => e
logger.error("DalliError: #{e.message}") if logger
raise if @raise_errors
raise if raise_errors?
false
end

# Delete an entry from the cache.
def delete_entry(key, options) # :nodoc:
@data.delete(key)
with_connection { |c| c.delete(key) }
rescue Dalli::DalliError => e
logger.error("DalliError: #{e.message}") if logger
raise if @raise_errors
raise if raise_errors?
false
end

Expand Down Expand Up @@ -321,6 +332,10 @@ def log(operation, key, options=nil)
return unless logger && logger.debug? && !silence?
logger.debug("Cache #{operation}: #{key}#{options.blank? ? "" : " (#{options.inspect})"}")
end

def raise_errors?
!!@options[:raise_errors]
end
end
end
end