ClickHouse / ClickHouse/clickhouse-connect

Support more granular timeouts for queries

Open
#192 6 comments 0 reactions 0 assignees View on GitHub
enhancement
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

Open the contributing 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

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.