feat: Try to generalize and refactor connection pool from workflows#692
Closed
ad-claw000 wants to merge 1 commit into
Closed
feat: Try to generalize and refactor connection pool from workflows#692ad-claw000 wants to merge 1 commit into
ad-claw000 wants to merge 1 commit into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a new ConnectionPool utility into the aperturedb Python SDK (ported/refactored from the workflows repo) and adds unit tests to validate basic pooling behavior.
Changes:
- Added
aperturedb/ConnectionPool.pyimplementing a queue-backed, context-manager-based connection pool. - Added
test/test_ConnectionPool.pywith unit tests for initialization, checkout/return, and query forwarding.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
aperturedb/ConnectionPool.py |
Adds the new ConnectionPool implementation and a convenience query() method. |
test/test_ConnectionPool.py |
Adds tests covering pool sizing, get_connection() context management, and query() forwarding. |
Comments suppressed due to low confidence (2)
aperturedb/ConnectionPool.py:83
query(self, query: str, blobs: list = [], ...)uses a mutable default ([]). If callers mutateblobs, that state can leak across calls. Preferblobs: Optional[list] = Noneand normalize to[]inside the method.
def query(self, query: str, blobs: list = [], **kwargs):
"""
aperturedb/ConnectionPool.py:60
- The pool swallows connection creation errors (prints and continues), but
_pool_sizeremains the requested size andtotal()reports that number even if fewer connections were actually created. Either fail fast when the pool cannot be fully populated, or track the actual number of created connections and havetotal()reflect reality (and/or expose both requested vs created).
# Pre-populate the pool with connections
for _ in range(pool_size):
try:
connection = self._connection_factory()
if not connection:
raise ConnectionError("Failed to create a connection.")
self._pool.put(connection)
except Exception as e:
print(f"Failed to create a connection for the pool: {e}")
# Depending on requirements, you might want to raise an error
# if the pool cannot be fully populated.
if self.available() == 0:
raise ConnectionError(
"Failed to initialize any connections for the pool. "
"Please check connection parameters and network."
)
def available(self) -> int:
"""Returns the number of available connections in the pool."""
return self._pool.qsize()
def total(self) -> int:
"""Returns the total number of connections in the pool."""
return self._pool_size
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+82
to
+99
| def query(self, query: str, blobs: list = [], **kwargs): | ||
| """ | ||
| A convenience method to execute a query directly from the pool. | ||
|
|
||
| This method handles getting a connection, executing the query, | ||
| and returning the connection to the pool. | ||
|
|
||
| Args: | ||
| query (str): The query string to execute. | ||
| blobs (list): A list of blobs to include with the query. | ||
| **kwargs: Other arguments for the Connector's query method. | ||
|
|
||
| Returns: | ||
| Response from the executed query. | ||
| Blobs | ||
| """ | ||
| with self.get_connection() as connection: | ||
| return connection.query(query, blobs, **kwargs) |
Comment on lines
+32
to
+34
| # A lock to ensure the initial population is thread-safe, just in case. | ||
| self._lock = threading.Lock() | ||
|
|
Comment on lines
+39
to
+43
| response, blobs = pool.query("some query", [1, 2, 3], arg=True) | ||
|
|
||
| assert response == "response" | ||
| assert blobs == [] | ||
| mock_connection.query.assert_called_once_with("some query", [1, 2, 3], arg=True) |
| @@ -0,0 +1,43 @@ | |||
| import pytest | |||
Contributor
|
there is already a pr for this #666 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #598