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

Dalli (memcached) adapter #132

Merged
merged 2 commits into from Jun 24, 2016

Conversation

Projects
None yet
2 participants
@AlexWheeler
Collaborator

AlexWheeler commented Jun 21, 2016

This is just initial thoughts on the memcached adapter #96 . Wanted to get something up and see if this similar to what you were thinking. Let me know, thanks! I don't think we would need to make a middleware for this like the memoization middleware. The user could just initialize their adapter with this adapter for example in a rails initializer when using rails. Or is there a reason we would want another middleware?

for example

# config/initializers/flipper.rb

active_record_adapter = Flipper::Adapters::ActiveRecord.new
adapter = Flipper::Adapters::Memcached.new(active_record_adapter)
$flipper = Flipper.new(adapter)

module Flipper
module Adapters
class Memcached

This comment has been minimized.

@jnunemaker

jnunemaker Jun 22, 2016

Owner

Since this is dalli and some apps use the memcached gem, I'd prefer to call this adapter dalli I think. Helps avoid confusing and allows for a memcached adapter down the road.

attr_accessor :cache
attr_accessor :name

def initialize(adapter, address = 'localhost:11211', options = {})

This comment has been minimized.

@jnunemaker

jnunemaker Jun 22, 2016

Owner

I'm kind of averse to taking parameters and then initializing an instance for people. The main problem I've ran into in the past is that these initialization options change and then I'm constantly updating or behind what is possible. Additionally, people usually have an instance already they are using in their app, so this forces multiple connections to be created. Instead I usually do something like this:

def initialize(adapter, cache)
  @adapter = adapter
  @cache = cache
end
require 'flipper/adapters/memory'
require 'flipper/adapters/memcached'

class MemcachedTest < MiniTest::Test

This comment has been minimized.

@jnunemaker

jnunemaker Jun 22, 2016

Owner

👍 Looks good here.


private

def cached(value)

This comment has been minimized.

@jnunemaker

jnunemaker Jun 22, 2016

Owner

I'd drop this method and use Client#fetch in dalli directly in the methods above. get/set can be racey as last write wins whereas get/add is not.

This comment has been minimized.

@jnunemaker

jnunemaker Jun 22, 2016

Owner

We'll probably want to provide a ttl option/argument for initialize that can be passed to fetch and perhaps default it to 0 or something.

end
end

def flush

This comment has been minimized.

@jnunemaker

jnunemaker Jun 22, 2016

Owner

I'd drop this method. Since we can pass in an instance, the instance can be used to flush in tests. New methods per adapter for adapters that are used in production can cause tricky issues (like using the method in middleware, then removing that adapter and then that method is gone and errors).

end

def add(feature)
@adapter.add(feature).tap do

This comment has been minimized.

@jnunemaker

jnunemaker Jun 22, 2016

Owner

Probably just personal preference, but I'm not a fan of tap really. I think my thought is there is no way a block/yield is faster than a temporary variable like this:

result = @adapter.add(feature)
@cache.delete(FeaturesKey)
result

I also think that it reads more explicitly for any programmer in any language as that is a common idiom, whereas tap might be known just by rubyists and what not. Not a huge concern there, but if things are more common, I don't mind if they are a bit less aesthetically beautiful.

Any passionate thoughts or feelings on keeping tap?

@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Jun 22, 2016

The user could just initialize their adapter with this adapter for example in a rails initializer when using rails.

Correct. This is what we do at GitHub for our memcached adapter.

Left some notes, but it looks super close!

@AlexWheeler AlexWheeler force-pushed the memcached-adapter branch 4 times, most recently from 9e9b38a to 2e47900 Jun 24, 2016

@AlexWheeler AlexWheeler force-pushed the memcached-adapter branch from 2e47900 to 9559f5f Jun 24, 2016

@AlexWheeler

This comment has been minimized.

Collaborator

AlexWheeler commented Jun 24, 2016

Thanks a ton for the comments @jnunemaker. They all were great and have been addressed.

@AlexWheeler AlexWheeler changed the title from Memcached adapter to Dalli (memcached) adapter Jun 24, 2016

# Public
def initialize(adapter, cache, ttl = 0)
@adapter = adapter
@name = adapter.name

This comment has been minimized.

@jnunemaker

jnunemaker Jun 24, 2016

Owner

I'm inclined to leave this as :dalli or :cached if we go that route. I think there is value in this adapter's name being different than the adapter it is wrapping so you can instrument them separately to see cached and uncached performance.

This comment has been minimized.

@jnunemaker
@jnunemaker

This comment has been minimized.

Owner

jnunemaker commented Jun 24, 2016

This looks great! So clean.

@jnunemaker jnunemaker merged commit 64f0295 into master Jun 24, 2016

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@jnunemaker jnunemaker deleted the memcached-adapter branch Jun 24, 2016

FeaturesKey = :flipper_features

#Internal
attr_accessor :cache

This comment has been minimized.

@jnunemaker

jnunemaker Jun 24, 2016

Owner

I also made these readers instead of accessors.

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