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

Add the ability to pass a Proc as the namespace #75

Closed
wants to merge 5 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@nberger
Contributor

nberger commented Jan 13, 2014

This is useful in scenarios where the namespace needs to dynamically change. For example in a multitenant app:

@namespaced = Redis::Namespace.new(Proc.new { Tenant.current.name }, :redis => @redis)
@tomaszgieres

This comment has been minimized.

tomaszgieres commented Jan 14, 2014

I'd like to have this in the gem.

@nberger

This comment has been minimized.

Contributor

nberger commented Jan 14, 2014

The build is broken, because all builds are broken. Merging #76 would fix the build :)

@nberger

This comment has been minimized.

Contributor

nberger commented Jun 7, 2014

This is totally broken :/, I'll open a new PR once I fix it

@nberger nberger closed this Jun 7, 2014

@nberger nberger reopened this Jun 8, 2014

@nberger

This comment has been minimized.

Contributor

nberger commented Jun 8, 2014

This was not broken, it was just the travis build :)

Rebased on master & merged #81 to have a green build. We can remove commits from #81 once the build is fixed on master.

By the way, is there interest on this PR? I'm using it in production in a multitenant app, it's working fine and I thought someone else could find it useful.

@yaauie

This comment has been minimized.

Member

yaauie commented Jun 19, 2014

@nberger I'm going to set aside a couple hours tonight to work on this & all of the others. Thanks for all your hard work. I had notifications disabled for this repo, so I missed out on the conversation 😦

@nberger

This comment has been minimized.

Contributor

nberger commented Jun 19, 2014

@yaauie no problem. Thanks to you. Let me know if you think something should be changed.

I'm keeping changes that do not belong here just to have a green build, we should remove all of that once master is fixed in travis.

@yaauie

This comment has been minimized.

Member

yaauie commented Jun 22, 2014

@nberger master is fixed and I'm taking a look at this.

While I appreciate the work, I'm leaning against accepting it.

The biggest problem is that debugging issues that arise out of incorrect application code (e.g., a proc is supplied that returns nil unexpectedly in some scenarios) will be significantly harder, and will likely result in significantly more issues opened on this library, which we can't handle.

It also opens up the door for significant issues, especially around concurrency. Consider:

When both input and output need keyspace handling (e.g., keys, blpop, mapped_mget), we call the proc on input and output, so a change in application state while a call is being made can cause odd corruptions.

When the output is an array or enumerator that needs keyspace removed (e.g., keys, scan, mapped_mget), in addition to the above problem, the proc is called for every item that is returned in order to remove the namespace, which represents a performance hit as well as opportunity for it not to return the same thing every time.

The concurrency problems could be addressed by caching the namespace used and using the same on the outbound processing, but even so, I'm having a hard time seeing the benefit that this change provides over explicitly defining the logic in application land.

Instead of:

def initialize(*args)
  # ...
  @namespaced_redis = Redis::Namespace.new(Proc.new { Tenant.current.name }, :redis => @redis)
end

# dynamically namespaces redis to the current Tenant.
attr_reader :namespaced_redis

Why not this, which keeps the logic closer to where it is happening:

# dynamically namespaces redis to the current Tenant.
def namespaced_redis
  Redis::Namespace.new(Tennant.current.name, :redis => @redis)
end

Thoughts?

@nberger

This comment has been minimized.

Contributor

nberger commented Jun 25, 2014

@yaauie thanks for your thoughtful answer.

I agree we should be very careful to not increase complexity and potential rate of support issues if this is not very worthy, or if there's an easy workaround. But I still think are some points in favor of this change:

  • I have the feeling that this is an advanced usage, so support issues shouldn't increase so much. If you are trying to namespace your redis connection and use a block to do that, probably you know what you are doing and know how to handle the concurrency. If the block result is not consistent with what's expected, you have serious issues. We can (and should) add a nice warning and explanation in the README if we are going to merge this.
  • With regard to application state changes that affect the block result, that shouldn't be an issue if the app handles the concurrency correctly. I'm thinking about a specific scenario: a multitenant app. In that case, it's easy to keep the state that affects the block as constant during a request cycle or during a background job run. It would be great to analyze this over a different scenario, but I don't have any that comes to my mind right now.
  • With regard to using something like you proposed:
# dynamically namespaces redis to the current Tenant.
def namespaced_redis
  Redis::Namespace.new(Tennant.current.name, :redis => @redis)
end

I think it's a good idea for many cases, but not when accessing the redis connection as a global shared resource. We might discuss if it's a good idea to manage resources as globally shared, but I think it's very common to access the redis connection as $redis or REDIS or @redis. Or think about something like sidekiq, where it's very common to setup the redis connection in an initializer and then forget about it, the block is very handy in cases like that.

Maybe I'm missing other scenarios, or I'm wrong in that the global resource is a common use case.

Let me know what you think.

@nberger

This comment has been minimized.

Contributor

nberger commented Jun 26, 2014

Rebased to master & fixed syntax for ruby 1.8.7

@yaauie

This comment has been minimized.

Member

yaauie commented Jun 26, 2014

Thanks for the rebase. As you can see I'm leaving the pull-request open, even though I think it will need to be implemented differently to address the above-mentioned concerns. I'll likely use your tests, but do some refactoring to the code to mitigate some of the issues I brought up, ensuring that the proc only gets called once per call to redis, etc.


While sharing a resource globally like $redis or within a class/module/object like @redis is commonplace, it also forces such libraries to maintain threadsafe interfaces (Redis::Namespace as yet has not had to, since it is able to rely on the threadsafety of the underlying Redis object), which is only really worth it if sharing (with the threadsafety overhead) is cheaper than creating new objects; while an actual redis connection would require a socket and is therefore worth the overhead needed to share, Redis::Namespace objects are super cheap to make and not overwhelmingly worth it, especially as you said that this is an advance feature so why spend the overhead when it's almost never needed?

But there's a fundamental difference between relying on a global $redis that is constantly connected to the same db (yeah, it supports select, which can cause problems) and relying on a dynamic wrapper that relies on externally-available application state that is only available at calltime that makes it inherently hard to test, hard to verify, and–when problems arise–hard to debug; at least with Redis#select, you explicitly have to tell it to change dbs, but here it is a side-effect of application state and that makes a big difference.

I find the idea of a dynamically-generated namespace intriguing. Here's another quick sprint on the subject, although it is just as susceptible to the concerns I posted in my initial rundown, or at least will be until the namespace is only #to_s'd once per call to redis.

class Dyn # forgive the short name
  def initialize(&block)
    @block = block
  end

  attr_reader :last

  def to_s
    @last = @block.call.to_s
  end
end
@namespaced = begin
  dynamic_namespace = Dyn.new { Tenant.current.name }
  Redis::Namespace.new(dynamic_namespace, redis: @redis)
end
@nberger

This comment has been minimized.

Contributor

nberger commented Jun 26, 2014

Cool. Feel free to take the parts that you think make sense. I'll do my best to be of help and keep learning :)

You are right in that creating a new Redis::Namespace instance is way cheaper than creating a new Redis connection, so I simplified over there... It's also nice to see the threadsafety guarantees that Redis::Namespace interface has to offer with respect to the underlying guarantees given by Redis. I named sidekiq, and as far as I remember I think sidekiq uses (or recommends using) ConnectionPool to be threadsafe with regard to acquiring the connection... Not directly related to this, but I'm seeing the thread-safety implications a little more now.

Liked the Dyn class proposal. As you said it doesn't address all the concerns, but it might be a path to extract the "dynamic" part from the Redis::Namespace class

@yaauie

This comment has been minimized.

Member

yaauie commented Aug 1, 2014

@nberger while I appreciate the work you put into this, I don't think this feature is something that I can spend the effort to maintain and support, so I'm closing the issue; I will however leave you with this, which requires 1.9+:

class Redis
  class DynamicNamespace < BasicObject
    # Uses the given block to generate a redis namespace dynamically
    # @param options [Hash{Symbol=>Object}]
    # @option options [Redis] :redis (Redis.current)
    # @yieldreturn [String]
    def initialize(options = {}, &namespace)
      fail ArgumentError, 'block required!' unless block_given?
      @namespace = namespace
      @options = options.dup
    end

    # @return [String]
    def current_namespace
      @namespace.call
    end

    # @api private
    def method_missing(*a,&b)
      Namespace.new(current_namespace, options).public_send(*a,&b)
    end
  end

  # monkey-patch Redis::Namespace to make it think that
  # DynamicNamespace is one of it
  def Namespace.===(other)
    super or other.kind_of?(DynamicNamespace)
  end
end

@yaauie yaauie closed this Aug 1, 2014

@nberger

This comment has been minimized.

Contributor

nberger commented Aug 1, 2014

Sounds fair. Thanks for the snippet 😄

@nberger nberger deleted the nberger:lambda-namespaces branch Sep 9, 2015

@nberger nberger restored the nberger:lambda-namespaces branch Sep 9, 2015

@fiedl

This comment has been minimized.

fiedl commented Nov 16, 2016

Hi @nberger, I've landed on this pull request page while searching a way to namespace my redis connections for the use with a multi-tenancy app. Did you ever solve this problem in a satisfactory way?

For multi-tenancy, I'm using the apartment gem.

Redis is used for sidekiq, redis-analytics and, most importantly, rails caching.

# config/initializers/cache.rb
Rails.application.config.cache_store = :redis_store, {
  host: ENV['REDIS_HOST'],
  port: '6379',
  expires_in: 1.week,
  namespace: "#{::STAGE}_cache",
  timeout: 15.0
}
Rails.cache = ActiveSupport::Cache.lookup_store(Rails.application.config.cache_store) # http://stackoverflow.com/a/38619281/2066546

# config/initializers/redis_analytics.rb
RedisAnalytics.configure do |configuration|
  configuration.redis_connection = Redis.new(host: ENV['REDIS_HOST'], port: '6379')
  configuration.redis_namespace = "#{::STAGE}_redis_analytics"
end

# config/initializers/sidekiq.rb
Sidekiq.configure_server do |config|
  config.redis = {host: ENV['REDIS_HOST'], port: '6379', namespace: "#{::STAGE}_sidekiq", timeout: 15.0 }
end
Sidekiq.configure_client do |config|
  config.redis = {host: ENV['REDIS_HOST'], port: '6379', namespace: "#{::STAGE}_sidekiq", timeout: 15.0 }
end

Thank you very much for any suggestion you might have!

EDIT: To make it easier for other users to find a possible solution, I've posted this as a question on stackoverflow: http://stackoverflow.com/q/40638628/2066546

@nberger

This comment has been minimized.

Contributor

nberger commented Nov 16, 2016

Hi @fiedl, it's been a long time since I last used this stuff. As far as I remember I just used my branch https://github.com/nberger/redis-namespace/tree/lambda-namespaces from which this PR was based on. It might need some updates because of deps changes, I haven't checked it out in a while. But of course feel free to use it.

Having said that, I remember that the approach proposed by yaauie in his last comment made much sense so I guess you could use consider it too.

fiedl added a commit to fiedl/your_platform that referenced this pull request Nov 18, 2016

namespacing the sidekiq connection
Sidekiq does not support `Proc` namespaces in its redis connection.

Therefore, we need to use (1) a `Redis::Namespace` wrapper class and
(2) a `Redis::DynamicNamespace` wrapper class, which allows Proc
namespaces by using `method_missing` to wrap the redis commands in a
namespace when calling the commands.

This is discussed here:
resque/redis-namespace#75

The `Redis::DynamicNamespace` wrapper is suggested here:
resque/redis-namespace#75 (comment)
resque/redis-namespace#75 (comment)

See also this stackoverflow question:
http://stackoverflow.com/q/40638628/2066546

Please note, this is just an intermediate step. Eventually, we will
move to separate redis server instances.

Internal trello cards:
https://trello.com/c/6xYBRb09/1075-redis-bottleneck
https://trello.com/c/cypH3vmW/896-multitenancy
@fiedl

This comment has been minimized.

fiedl commented Nov 18, 2016

Thanks for your suggestions, @nberger and @yaauie! Using Redis::DynamicNamespace actually worked pretty well.

I think, however, we will eventually move to using separate redis server instances, mainly for performance reasons.

fiedl added a commit to fiedl/your_platform that referenced this pull request Nov 18, 2016

namespacing redis_analytics
redis analytics does not support Proc namespaces in its redis
connection.

Thus, we need to follow the same procedure as for sidekiq:
cef3620
8ff2b0a4d05d11

Discussion: resque/redis-namespace#75

See also this stackoverflow question:
http://stackoverflow.com/q/40638628/2066546

Please note, this is just an intermediate step. Eventually, we will
move to separate redis server instances.

Internal trello cards:
https://trello.com/c/6xYBRb09/1075-redis-bottleneck
https://trello.com/c/cypH3vmW/896-multitenancy

fiedl added a commit to fiedl/your_platform that referenced this pull request Feb 21, 2017

namespacing the sidekiq connection
Sidekiq does not support `Proc` namespaces in its redis connection.

Therefore, we need to use (1) a `Redis::Namespace` wrapper class and
(2) a `Redis::DynamicNamespace` wrapper class, which allows Proc
namespaces by using `method_missing` to wrap the redis commands in a
namespace when calling the commands.

This is discussed here:
resque/redis-namespace#75

The `Redis::DynamicNamespace` wrapper is suggested here:
resque/redis-namespace#75 (comment)
resque/redis-namespace#75 (comment)

See also this stackoverflow question:
http://stackoverflow.com/q/40638628/2066546

Please note, this is just an intermediate step. Eventually, we will
move to separate redis server instances.

Internal trello cards:
https://trello.com/c/6xYBRb09/1075-redis-bottleneck
https://trello.com/c/cypH3vmW/896-multitenancy

(cherry picked from commit cef3620)

fiedl added a commit to fiedl/your_platform that referenced this pull request Feb 21, 2017

namespacing redis_analytics
redis analytics does not support Proc namespaces in its redis
connection.

Thus, we need to follow the same procedure as for sidekiq:
cef3620
8ff2b0a4d05d11

Discussion: resque/redis-namespace#75

See also this stackoverflow question:
http://stackoverflow.com/q/40638628/2066546

Please note, this is just an intermediate step. Eventually, we will
move to separate redis server instances.

Internal trello cards:
https://trello.com/c/6xYBRb09/1075-redis-bottleneck
https://trello.com/c/cypH3vmW/896-multitenancy

(cherry picked from commit 9396e36)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment