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

Conversation

jcxplorer
Copy link
Contributor

As promised, here's a pull request that makes DalliStore a little easier to subclass, for example, to add support for a connection pool.

I took an approach that adds a #with_connection method through which the Dalli client is always accessed. It seemed like the best way to support a connection pool in a subclass, which can be done using the connection_pool gem:

module ActiveSupport
  module Cache
    class DalliPoolStore < DalliStore

      def initialize(pool, options={})
        @pool = pool
        @options = options
        extend Strategy::LocalCache
      end

      def with_connection(&block)
        @pool.with(&block)
      end

    end
  end
end
dalli_pool = ConnectionPool.new(size: 25, timeout: 5) { Dalli::Client.new("localhost:11211", threadsafe: false) }
config.cache_store = :dalli_pool_store, dalli_pool

Couple questions that popped up while I was doing this:

  1. DalliStore#dalli is not used anywhere and the tests don't seem to mind if it's not defined. Was it used internally at some point or is it just to provide access to a Dalli::Client instance via Rails.cache.dalli? That method won't work with the example pool store above, but I think that's ok.
  2. DalliStore#read_multi calls extract_options! on its arguments but doesn't assign its return value anywhere unlike other methods such as #fetch_multi. There's an options variable being used later which ends up being the options passed when the DalliStore is created. Is this intentional or a bug? I'm not familiar enough with the code to be able to tell :)

memo[name] = value
write_entry(expanded, value, options)
end
results = with_connection { |c| c.get_multi(mapping.keys) }
Copy link
Collaborator

Choose a reason for hiding this comment

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

You don't need to call with_connection twice on these two lines. Reuse the connection.

@jcxplorer
Copy link
Contributor Author

Here it is. Could do the same for read_entry and delete_entry but all places that call those don't have an open connection, so I'm not sure if it's worth the trouble.

I can squash those commits once you think it's good to go.

@mperham
Copy link
Collaborator

mperham commented Jan 4, 2014

Good enough, squash away!

@jcxplorer
Copy link
Contributor Author

All done, thanks a lot for reviewing this!

mperham added a commit that referenced this pull request Jan 4, 2014
Refactor DalliStore to better support subclassing
@mperham mperham merged commit a10ad1b into petergoldstein:master Jan 4, 2014
@mperham
Copy link
Collaborator

mperham commented Jan 6, 2014

I've added connection pool support into the standard dalli_store adapter. See c51ce5d

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.

None yet

2 participants