-
Notifications
You must be signed in to change notification settings - Fork 211
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
Should memoization be used? #147
Comments
We noticed this as well and ended up subclassing the class MemoizedRollout < Rollout
def active?(feature, user = nil)
key = key_for(feature, user)
RequestStore.fetch(key) { super }
end
def activate(feature)
clear_cache(feature)
super
end
def deactivate(feature)
clear_cache(feature)
super
end
def activate_user(feature, user)
clear_cache(feature)
super
end
def deactivate_user(feature, user)
clear_cache(feature)
super
end
private
def clear_cache(feature)
RequestStore.store.keys.each do |key|
next unless key.to_s.start_with?("rollout:#{feature}")
RequestStore.delete(key)
end
end
def key_for(feature, user)
['rollout', feature.to_s, user&.class&.to_s, user&.id].compact.join(':')
end
end |
Cool! @jordanfbrown, thanks for sharing! |
Rollout isn’t overly public about it but I believe it uses the adapter pattern by only using like two of Redis methods. You can inject anything you like which means you can make a class with same interface that wraps the Redis client with memorization. |
Kind what @jordanfbrown did but you shouldn’t need to subclass rollout. You can just define the same interface and initialize with a Redis instance. Your class could be MemoizedRedis. Then you just pass that into Rollout.new. The only other issue with memoization is to turn it on per request or job and ensure it is cleared after. You can do this with middleware or in before action calls in rails. Middleware early in the stack is best. |
@jnunemaker Ha thanks! I was just thinking about how https://github.com/jnunemaker/flipper works when opened the issue here |
Description
I found the feature setting is retrieved directly from redis often if used on every request. Should we memoize it?
The text was updated successfully, but these errors were encountered: