-
Notifications
You must be signed in to change notification settings - Fork 162
add ConnectionPool.autoconnect #147
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,10 +48,14 @@ class ConnectionPool(object): | |
task of the pool. | ||
|
||
:param int size: the maximum number of concurrently open connections | ||
: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. | ||
: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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you add a |
||
if not isinstance(size, int): | ||
raise TypeError("Pool 'size' arg must be an integer") | ||
|
||
|
@@ -72,11 +76,12 @@ def __init__(self, size, **kwargs): | |
connection = Connection(**connection_kwargs) | ||
self._queue.put(connection) | ||
|
||
# The first connection is made immediately so that trivial | ||
# mistakes like unresolvable host names are raised immediately. | ||
# Subsequent connections are connected lazily. | ||
with self.connection(): | ||
pass | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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." |
||
with self.connection(): | ||
pass | ||
|
||
def _acquire_connection(self, timeout=None): | ||
"""Acquire a connection from the pool.""" | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,6 +16,7 @@ | |
assert_false, | ||
assert_in, | ||
assert_is_instance, | ||
assert_is_none, | ||
assert_is_not_none, | ||
assert_list_equal, | ||
assert_not_in, | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what are you trying to test here? it is not obvious to me :) |
||
|
||
def test_connection_pool(): | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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