Copilot commented on code in PR #7300:
URL: https://github.com/apache/kyuubi/pull/7300#discussion_r2679590190
##########
python/pyhive/hive.py:
##########
@@ -160,7 +160,8 @@ def __init__(
check_hostname=None,
ssl_cert=None,
thrift_transport=None,
- ssl_context=None
+ ssl_context=None,
+ connection_timeout=None,
Review Comment:
The connection_timeout parameter should be validated to ensure it's a
positive value. Negative or zero timeout values could cause unexpected
behavior. Consider adding validation like:
```python
if connection_timeout is not None and connection_timeout <= 0:
raise ValueError("connection_timeout must be a positive value")
```
##########
python/pyhive/tests/test_hive.py:
##########
@@ -259,6 +259,17 @@ def test_basic_ssl_context(self):
cursor.execute('SELECT 1 FROM one_row')
self.assertEqual(cursor.fetchall(), [(1,)])
+ def test_connection_timeout(self):
+ """Test that a connection timeout is set without error."""
+ with contextlib.closing(hive.connect(
+ host=_HOST,
+ port=10000,
+ connection_timeout=10 * 1000
+ )) as connection:
+ with contextlib.closing(connection.cursor()) as cursor:
+ # Use the same query pattern as other tests
+ cursor.execute('SELECT 1 FROM one_row')
+ self.assertEqual(cursor.fetchall(), [(1,)])
Review Comment:
The test only verifies that a connection can be established with the timeout
parameter, but doesn't actually test timeout behavior. Consider adding a test
that verifies the timeout is properly set on the underlying socket/transport.
This could be done by checking the socket's timeout value after connection, or
by testing that a connection attempt to an unreachable host times out as
expected.
##########
python/pyhive/hive.py:
##########
@@ -175,6 +176,7 @@ def __init__(
Incompatible with host, port, auth, kerberos_service_name, and
password.
:param ssl_context: A custom SSL context to use for HTTPS connections.
If provided,
this overrides check_hostname and ssl_cert parameters.
+ :param connection_timeout: Millisecond timeout for Thrift connections.
Review Comment:
The documentation should clarify that connection_timeout is ignored when
using a custom thrift_transport. Users need to configure the timeout on their
custom transport object directly. Consider adding a note to the docstring like:
"Note: connection_timeout is ignored when thrift_transport is provided.
Configure timeout on the custom transport object instead."
```suggestion
:param connection_timeout: Millisecond timeout for Thrift
connections when using the
built-in HTTP/HTTPS transport.
Note: connection_timeout is ignored when thrift_transport is
provided. Configure
timeout on the custom transport object instead.
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
