ClickHouse / ClickHouse/clickhouse-connect
Support more granular timeouts for queries
- Dominant language
- Python
- Stars
- 521
- Forks
- 159
- Avg merge
- 1d 8h
- Merged PRs (30d)
- 43
Description
**Is your feature request related to a problem? Please describe.**
Feature request / enhancement
**Describe the solution you'd like**
I'd like to have more granular control over timeouts in the client. Specifically, the `ping` method has a [hardcoded timeout](https://github.com/ClickHouse/clickhouse-connect/blob/fa20547d7f7e2fd3a2cf4cd711c3262c5a79be7a/clickhouse_connect/driver/httpclient.py#L414) of `3` but in our use case we'd like to be able to configure that to something like `1` for example.
- The `clickhouse-driver` library [solves this](https://clickhouse-driver.readthedocs.io/en/latest/api.html#connection) using a `sync_request_timeout` argument. Perhaps something similar could be added to the client constructor here.
The `_raw_request` method [uses the timeout](https://github.com/ClickHouse/clickhouse-connect/blob/fa20547d7f7e2fd3a2cf4cd711c3262c5a79be7a/clickhouse_connect/driver/httpclient.py#L361) on the client instance, which is more flexible, but our use case requires a per-query timeout setting. Currently, I'm working around this with a solution I don't love
```python3
@contextmanager
def with_timeout(
self,
connect_timeout: Optional[int] = None,
send_receive_timeout: Optional[int] = None,
):
_old_timeout = self._delegate.timeout
self._delegate.timeout = Timeout(
connect=coerce_int(connect_timeout or _old_timeout.connect_timeout),
read=coerce_int(send_receive_timeout or _old_timeout.read_timeout),
)
yield self._delegate
self._delegate.timeout = _old_timeout
```
This approach is similar to that in `clickhouse-driver` which offers a `timeout_setter` on the `Connection` instance it provides. A builtin way to supply a timeout either through a context manager or with a sort of per-query setting e.g. `client.query(..., connect_timeout=1, send_receive_timeout=3)` would be great.
**Describe alternatives you've considered**
Right now I'm composing a class that adds some workarounds (the `with_timeout` above) and for ping I override the method
```python3
def ping(self):
"""
Ping the host, sync_request_timeout is supplied to the call
TODO: Suggest this as a PR to clickhouse-connect
"""
try:
response = self._delegate.http.request(
"GET", f"{self._delegate.url}/ping", timeout=self._sync_request_timeout
)
return 200 <= response.status < 300
except HTTPError:
return False
```
I suppose query context or settings could also be a feasible way to supply the timeout. I don't have a strong opinion on how it should be accomplished and would love to hear your thoughts.
**Additional context**
I'm happy to PR any of these changes, just wanted to start a discussion on the approach before starting any implementation.
Contributor guide
Research direction
Start in clickhouse_connect/driver/httpclient.py at ping and _raw_request, then trace the client query entry points. Review the clickhouse-driver sync_request_timeout API and the issue's context-manager workaround before settling on the supported configuration shape. Done means the agreed design provides configurable ping and per-query timeouts without requiring method overrides.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- python
- Domain
- backend-api-design
- Issue type
- Feature
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Active
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100