add ConnectionPool.autoconnect #147
Conversation
| :param bool autoconnect: Whether a connection should be created upon | ||
| instantiation to test connection availability. | ||
| Note: the `autoconnect` flag of Connection instance inside the pool | ||
| is always False. |
There was a problem hiding this comment.
Can you rephrase the second part as "Note that this is unrelated to the autoconnect argument for connections, which is not applicable to connections managed by a connection pool." ? thanks
| :param kwargs: keyword arguments passed to | ||
| :py:class:`happybase.Connection` | ||
| """ | ||
| def __init__(self, size, **kwargs): | ||
| def __init__(self, size, autoconnect=True, **kwargs): |
There was a problem hiding this comment.
can you add a since comment (see other docstrings) to make clear when this api was added/changed?
| if autoconnect: | ||
| # The first connection is made immediately so that trivial | ||
| # mistakes like unresolvable host names are raised immediately. | ||
| # Subsequent connections are connected lazily. |
There was a problem hiding this comment.
nitpick: can you change this into
"Immediately open one connection so that trivial mistakes like unresolvable host names raise an exception upon pool instantiation instead of on first use. Subsequent connections are connected lazily."
| @@ -487,6 +488,9 @@ def test_connection_pool_construction(): | |||
| with assert_raises(ValueError): | |||
| ConnectionPool(size=0) | |||
|
|
|||
| pool = ConnectionPool(size=1, autoconnect=False) | |||
| assert_is_none(getattr(pool._thread_connections, 'current')) | |||
|
|
|||
There was a problem hiding this comment.
what are you trying to test here? it is not obvious to me :)
|
@luyun-aa is this still relevant? if so, please address my comments, and update the pr. thanks for your contribution. |
@wbolster According to discussion in #143, this PR adds a
autoconnectargument to ConnectionPool constructor.