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

Expose Redis connection pool #12

Open
cmelbye opened this issue Jan 18, 2014 · 5 comments
Open

Expose Redis connection pool #12

cmelbye opened this issue Jan 18, 2014 · 5 comments

Comments

@cmelbye
Copy link

cmelbye commented Jan 18, 2014

Is there any way that the Redis connection pool could be exposed to worker functions somehow? It's a slight inconvenience to have to set up a separate connection pool, reimplement configuration logic, etc. It'd be great to be able to easily ".Get" a connection from the pool to use.

Not sure if this is something wanted for this project though.

@oliamb
Copy link

oliamb commented Jan 20, 2014

In issue #11, I tried to extend the lib with an Enqueue method, but there is a problem with the pool management:
I cannot guess when I can close it easily, because the original code close the pool when there is no more worker.
In fact I had to disable this but I cannot sleep anymore since then :-)

Sharing a pool is a very important feature I think because you want to be able to define the number of available connections for the whole application, not per library.

The problem should be solved by doing the opposite, there should be a way to pass a connection pool to Goworker. That the role of the application to manage it, not from the library.

What do you think?

@charl
Copy link

charl commented Jan 20, 2014

@oliamb injecting your own redis pool sound great as long as it implements an interface that guarantees the operations goworker expects, exist in the custom worker pool.

There is however an inherent risk here sharing the pool between goworker and the client app/workers in that either one could in theory monopolise the redis connections and DoS the other. For this reason it may be better to keep them separate.

@oliamb
Copy link

oliamb commented Jan 20, 2014

First, I should state that the actual behavior should be kept for the sake of simplicity. Passing the pool should be considered optional. From there, that's up to the developer to decide whether he trust his workers enough to share a common pool or if he wants to have different pools for different parts.

  • DoS: yes, true. I had this concern about the Enqueue func. It seems that at least goworker do it right by pushing the connection back to the pool before processing the job, so even a pool containing one connection will not run into some kind of dead lock ; as long as the job also push the connection back as well.
  • Implementing the right interface: true again, this is a somewhat hard dependency on redigo and code.google.com/p/vitess/go/pools, but probably it is ok?

@charl
Copy link

charl commented Jan 20, 2014

@oliamb so you'd only be able to inject a vitess/redigo pool for now?

That's fine as it changes less code.

If someone needs to use another pool/redis client the option still remains to insert an interface into goworker that all pools need to conform to. This can be built when the need arises though.

@cmelbye
Copy link
Author

cmelbye commented Jan 20, 2014

That makes sense to me. I'd be fine with making my own redigo pool and then giving that to goworker to use.

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 a pull request may close this issue.

3 participants