Riak ConnectionManager implemented with gevent.queue.Queue - #142
Riak ConnectionManager implemented with gevent.queue.Queue#142ixmatus wants to merge 22 commits into
Conversation
…ager to a gevent queue
There was a problem hiding this comment.
Is it strictly necessary that the pool be the same size as the list of hosts? M:N seems perfectly reasonable to me.
There was a problem hiding this comment.
No it is not necessary - if you think M:N is an acceptable way to handle it I can do it that way; problem with providing a pool size of "10" could be that if you have a four node list (or larger) - that's 40 connections if the default pool_size isn't changed. Which isn't bad per-se but that many connections seems like over-kill to me?
Maybe I'm being too conservative with that...
There was a problem hiding this comment.
Oh, i misread the code. The exception message should be rewritten to say "hostports cannot be larger than pool_size". That makes more sense, but maybe it should still be pool_size * len(hostports), where pool_size is small. That way you don't get certain hosts arbitrarily preferred over others.
|
I'd be curious if there was a way to make this more "optional". As I understand it, this PR would now require gevent and would monkey patch socket, which affects all other python libraries used in the application. Also, for a database client library like this, what's wrong with just using Python threads? |
|
@reiddraper I agree the requirement for gevent is somewhat shaky as it then requires libevent to be installed. The good news with gevent 2.x is it comes bundled with libev; so that would be a bit more tenable. But that is still in Beta as it is. My rationale for going with gevent was for the lightweight thread approach; seemed more attractive to me. However you could very well be correct in that using straight up Python threads would be the more elegant but boring approach. |
|
@ixmatus boring is OK with me :). I'd suspect for the number of threads we're talking about (less than a couple hundred), Python threads are just fine. |
|
@ixmatus Sorry for leaving this hanging for a while, but I don't think that we want gevent as a dependency at the moment. #157 changes this code around some, which might change your target a little. Do you think that it might be possible to refactor the code such that it detects and uses gevent if it's importable (presumably by the user's choice) and either falls back to the default or to a plain threads implementation? |
|
@evanmcc I've left it hanging TBH - I've been terribly busy with my startup; I do want to move it to using the standard Python threadpool. No need to detect gevent honestly; I had a moment of wanting to play with gevent and it's clear it isn't well suited for this application :-p In a few nights I might spend some time reworking this. |
I tweaked the ConnectionManager API a little bit, used gevent's Queue (riak client operations are now concurrent, thread-safe, and much more lightweight).
The RiakClient args have been shuffled a little bit to make more sense:
To create a connection pool for just one host (default number is 10 connections):
client = riak.RiakClient(host=("10.8.0.1", 8087), transport_class=riak.RiakPbcTransport)
To create a connection pool for multiple hosts:
client = riak.RiakClient(host=[("10.8.0.1", 8087), (10.8.0.2", 8087)], pool_size=2, transport_class=riak.RiakPbcTransport)
Please note: unit tests have not been written for this new functionality and it DOES break backwards compatibility. Even if a pull isn't accepted it may (hopefully) give someone at Basho something to work with in using gevent for the connection pooler.