Skip to content

Commit 34d3e39

Browse files
amotlbgunebakanseut
authored
Fix: Fail early when database cluster does not respond (#711)
* Error handling: Fail early when database cluster does not respond * Error handling: Only re-raise ConnectionError when all servers fail The procedure will collect all `ConnectionError` instances and include them into the exception message of the final `ConnectionError`. * Error handling: Use JSON for bundling multiple ConnectionError instances All `ConnectionError` instances that have been collected will be serialized into JSON now. * fix linter errors * Fix test suite compatibility with fail-on-connect behavior change * Improve error handling in _lowest_server_version and add tests for connection errors * Update docs with new changes * Apply suggestions from code review Co-authored-by: Sebastian Utz <su@rtme.net> --------- Co-authored-by: Bilal Tonga <bilaltonga@gmail.com> Co-authored-by: Sebastian Utz <su@rtme.net>
1 parent bacebe5 commit 34d3e39

6 files changed

Lines changed: 219 additions & 28 deletions

File tree

CHANGES.rst

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@
22
Changes for crate
33
=================
44

5+
Unreleased
6+
================
7+
8+
- Breaking change: ``connect()`` now raises ``ConnectionError`` immediately if
9+
no configured server node responds.
10+
511
2026/06/17 2.2.1
612
================
713

docs/by-example/client.rst

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -29,12 +29,8 @@ respond, the request is automatically routed to the next server:
2929
>>> connection = client.connect([invalid_host, crate_host])
3030
>>> connection.close()
3131

32-
If no ``servers`` are given, the default one ``http://127.0.0.1:4200`` is used:
33-
34-
>>> connection = client.connect()
35-
>>> connection.client._active_servers
36-
['http://127.0.0.1:4200']
37-
>>> connection.close()
32+
If no ``servers`` are supplied to the ``connect`` method, the default address
33+
``http://127.0.0.1:4200`` is used.
3834

3935
If the option ``error_trace`` is set to ``True``, the client will print a whole
4036
traceback if a server error occurs:
@@ -77,7 +73,7 @@ connect:
7773

7874
The username for trusted users can also be provided in the URL:
7975

80-
>>> connection = client.connect(['http://trusted_me@' + crate_host])
76+
>>> connection = client.connect([crate_host.replace('://', '://trusted_me@')])
8177
>>> connection.client.username
8278
'trusted_me'
8379
>>> connection.client.password
@@ -97,7 +93,7 @@ also need to provide ``password`` as argument for the ``connect()`` call:
9793

9894
The authentication credentials can also be provided in the URL:
9995

100-
>>> connection = client.connect(['http://me:my_secret_pw@' + crate_host])
96+
>>> connection = client.connect([crate_host.replace('://', '://me:my_secret_pw@')])
10197
>>> connection.client.username
10298
'me'
10399
>>> connection.client.password

docs/connect.rst

Lines changed: 20 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,13 +73,32 @@ You can pass in as many node URLs as you like.
7373

7474
.. TIP::
7575

76-
For every query, the client will attempt to connect to each node in sequence
76+
When ``connect()`` is called, the client contacts each node to check
77+
availability and determine the lowest server version. If no node responds,
78+
a ``ConnectionError`` is raised immediately.
79+
80+
For every subsequent query, the client will attempt each node in sequence
7781
until a successful connection is made. Nodes are moved to the end of the
7882
list each time they are tried.
7983

8084
Over multiple query executions, this behaviour functions as client-side
8185
*round-robin* load balancing. (This is analogous to `round-robin DNS`_.)
8286

87+
.. NOTE::
88+
89+
Wrap ``connect()`` in a ``try/except`` block to handle an unreachable
90+
cluster gracefully:
91+
92+
.. code-block:: python
93+
94+
from crate import client
95+
from crate.client.exceptions import ConnectionError
96+
97+
try:
98+
connection = client.connect(["node-1:4200", "node-2:4200"])
99+
except ConnectionError as e:
100+
print(f"Could not reach CrateDB cluster: {e}")
101+
83102
.. _connection-options:
84103

85104
Connection options

src/crate/client/connection.py

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@
1818
# However, if you have executed another commercial license agreement
1919
# with Crate these terms will supersede the license and you may use the
2020
# software solely pursuant to the terms of the relevant commercial agreement.
21-
2221
from typing import Union
2322

2423
from verlib2 import Version
@@ -211,14 +210,21 @@ def get_blob_container(self, container_name):
211210

212211
def _lowest_server_version(self):
213212
lowest = None
214-
for server in self.client.active_servers:
213+
servers = self.client.active_servers
214+
connection_errors = []
215+
for server in servers:
215216
try:
216217
_, _, version = self.client.server_infos(server)
217218
version = Version(version)
218-
except (ValueError, ConnectionError):
219+
except ConnectionError as ex:
220+
connection_errors.append(ex)
221+
continue
222+
except ValueError:
219223
continue
220224
if not lowest or version < lowest:
221225
lowest = version
226+
if connection_errors and not lowest:
227+
raise ConnectionError("; ".join(str(e) for e in connection_errors))
222228
return lowest or Version("0.0.0")
223229

224230
def __repr__(self):

tests/client/test_connection.py

Lines changed: 164 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@
44
import pytest
55
from urllib3 import Timeout
66

7+
import crate.client.exceptions
78
from crate.client import connect
89
from crate.client.connection import Connection
910
from crate.client.exceptions import ProgrammingError
@@ -12,6 +13,40 @@
1213
from .settings import crate_host
1314

1415

16+
class _FakeClient:
17+
"""
18+
Minimal stand-in for Client that lets tests control server_infos.
19+
"""
20+
21+
def __init__(self, servers, server_infos_fn):
22+
self._servers = list(servers)
23+
self._server_infos_fn = server_infos_fn
24+
25+
@property
26+
def active_servers(self):
27+
return list(self._servers)
28+
29+
def server_infos(self, server):
30+
return self._server_infos_fn(server)
31+
32+
33+
def _bare_conn(client):
34+
"""
35+
Create a Connection that bypasses __init__.
36+
"""
37+
38+
conn = Connection.__new__(Connection)
39+
conn.client = client
40+
return conn
41+
42+
43+
def test_invalid_server_address():
44+
client = Client(servers="localhost:1234")
45+
with pytest.raises(crate.client.exceptions.ConnectionError) as excinfo:
46+
connect(client=client)
47+
assert excinfo.match("Server not available")
48+
49+
1550
def test_lowest_server_version():
1651
"""
1752
Verify the lowest server version is correctly set.
@@ -55,10 +90,13 @@ def test_connection_closes_access():
5590

5691
def test_connection_closes_context_manager():
5792
"""Verify that the context manager of the client closes the connection"""
58-
with patch.object(connect, "close", autospec=True) as close_fn:
59-
with connect():
60-
pass
61-
close_fn.assert_called_once()
93+
with patch.object(
94+
Client, "server_infos", return_value=(None, None, "0.0.0")
95+
):
96+
with patch.object(connect, "close", autospec=True) as close_fn:
97+
with connect():
98+
pass
99+
close_fn.assert_called_once()
62100

63101

64102
def test_invalid_server_version():
@@ -78,8 +116,11 @@ def test_context_manager():
78116
"""
79117
close_method = "crate.client.http.Client.close"
80118
with patch(close_method, return_value=MagicMock()) as close_func:
81-
with connect("localhost:4200") as conn:
82-
assert not conn._closed
119+
with patch.object(
120+
Client, "server_infos", return_value=(None, None, "0.0.0")
121+
):
122+
with connect("localhost:4200") as conn:
123+
assert not conn._closed
83124

84125
assert conn._closed
85126
# Checks that the close method of the client
@@ -115,7 +156,10 @@ def test_default_repr():
115156
"""
116157
Verify default repr dunder method.
117158
"""
118-
conn = connect()
159+
with patch.object(
160+
Client, "server_infos", return_value=(None, None, "0.0.0")
161+
):
162+
conn = connect()
119163
assert repr(conn) == "<Connection <Client ['http://127.0.0.1:4200']>>"
120164

121165

@@ -132,7 +176,10 @@ def test_with_timezone():
132176
"""
133177

134178
tz_mst = datetime.timezone(datetime.timedelta(hours=7), name="MST")
135-
connection = connect("localhost:4200", time_zone=tz_mst)
179+
with patch.object(
180+
Client, "server_infos", return_value=(None, None, "0.0.0")
181+
):
182+
connection = connect("localhost:4200", time_zone=tz_mst)
136183
cursor = connection.cursor()
137184

138185
assert cursor.time_zone.tzname(None) == "MST"
@@ -148,22 +195,125 @@ def test_timeout_float():
148195
"""
149196
Verify setting the timeout value as a scalar (float) works.
150197
"""
151-
with connect("localhost:4200", timeout=2.42) as conn:
152-
assert conn.client._pool_kw["timeout"] == 2.42
198+
with patch.object(
199+
Client, "server_infos", return_value=(None, None, "0.0.0")
200+
):
201+
with connect("localhost:4200", timeout=2.42) as conn:
202+
assert conn.client._pool_kw["timeout"] == 2.42
153203

154204

155205
def test_timeout_string():
156206
"""
157207
Verify setting the timeout value as a scalar (string) works.
158208
"""
159-
with connect("localhost:4200", timeout="2.42") as conn:
160-
assert conn.client._pool_kw["timeout"] == 2.42
209+
with patch.object(
210+
Client, "server_infos", return_value=(None, None, "0.0.0")
211+
):
212+
with connect("localhost:4200", timeout="2.42") as conn:
213+
assert conn.client._pool_kw["timeout"] == 2.42
161214

162215

163216
def test_timeout_object():
164217
"""
165218
Verify setting the timeout value as a Timeout object works.
166219
"""
167220
timeout = Timeout(connect=2.42, read=0.01)
168-
with connect("localhost:4200", timeout=timeout) as conn:
169-
assert conn.client._pool_kw["timeout"] == timeout
221+
with patch.object(
222+
Client, "server_infos", return_value=(None, None, "0.0.0")
223+
):
224+
with connect("localhost:4200", timeout=timeout) as conn:
225+
assert conn.client._pool_kw["timeout"] == timeout
226+
227+
228+
def test_partial_failure_raises():
229+
"""
230+
When some servers fail with ConnectionError and others produce an
231+
unparseable version string (triggering ValueError/InvalidVersion),
232+
the method must still raise rather than silently returning Version("0.0.0").
233+
234+
Risk: len(connection_errors) < server_count because only ConnectionError
235+
instances are counted, so the all-failed guard never fires.
236+
"""
237+
238+
def server_infos(server):
239+
if "4200" in server:
240+
raise crate.client.exceptions.ConnectionError(
241+
"Server not available"
242+
)
243+
# "bad-version" triggers InvalidVersion inside Version(), which is
244+
# caught by the second except branch and never appended to
245+
# connection_errors.
246+
return (None, None, "bad-version")
247+
248+
client = _FakeClient(
249+
["http://localhost:4200", "http://localhost:4201"],
250+
server_infos,
251+
)
252+
conn = _bare_conn(client)
253+
254+
with pytest.raises(crate.client.exceptions.ConnectionError):
255+
conn._lowest_server_version()
256+
257+
258+
def test_error_message_contains_individual_errors():
259+
"""
260+
When all servers fail with ConnectionError the raised exception message
261+
must contain each individual server's error text so operators can see
262+
which nodes are down.
263+
"""
264+
msgs = {
265+
"http://localhost:4200": "node-A refused connection",
266+
"http://localhost:4201": "node-B timed out",
267+
}
268+
269+
def server_infos(server):
270+
raise crate.client.exceptions.ConnectionError(msgs[server])
271+
272+
client = _FakeClient(list(msgs), server_infos)
273+
conn = _bare_conn(client)
274+
275+
with pytest.raises(crate.client.exceptions.ConnectionError) as excinfo:
276+
conn._lowest_server_version()
277+
278+
msg = str(excinfo.value)
279+
assert "node-A refused connection" in msg
280+
assert "node-B timed out" in msg
281+
282+
283+
def test_active_servers_double_evaluation():
284+
"""
285+
active_servers is evaluated twice: once for len() (server_count) and once
286+
for the for-loop. If more servers appear between the two calls, every
287+
iterated server can fail with ConnectionError yet len(connection_errors)
288+
exceeds the stale server_count, causing the all-failed guard to miss.
289+
290+
"""
291+
292+
class _UnstableClient:
293+
def __init__(self):
294+
self._calls = 0
295+
296+
@property
297+
def active_servers(self):
298+
self._calls += 1
299+
if self._calls == 1:
300+
# First access: len() call — reports 2 servers.
301+
return ["http://localhost:4200", "http://localhost:4201"]
302+
# Second access: for-loop — a third server appeared concurrently.
303+
return [
304+
"http://localhost:4200",
305+
"http://localhost:4201",
306+
"http://localhost:4202",
307+
]
308+
309+
def server_infos(self, server):
310+
raise crate.client.exceptions.ConnectionError(
311+
"Server not available"
312+
)
313+
314+
conn = _bare_conn(_UnstableClient())
315+
316+
# All 3 iterated servers fail, but server_count=2 (stale).
317+
# 3 != 2 → guard never fires → silently returns Version("0.0.0").
318+
with pytest.raises(crate.client.exceptions.ConnectionError):
319+
conn._lowest_server_version()

tests/client/test_http.py

Lines changed: 16 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -655,7 +655,14 @@ def do_POST(self):
655655
time.sleep(timeout + 0.1)
656656

657657
def do_GET(self):
658-
pass
658+
body = json.dumps(
659+
{"name": "test", "version": {"number": "0.0.0"}}
660+
).encode()
661+
self.send_response(200)
662+
self.send_header("Content-Type", "application/json")
663+
self.send_header("Content-Length", str(len(body)))
664+
self.end_headers()
665+
self.wfile.write(body)
659666

660667
# Start the http server.
661668
with serve_http(TimeoutRequestHandler) as (server, url):
@@ -710,7 +717,14 @@ def do_POST(self):
710717
self.wfile.write(response.encode("utf-8"))
711718

712719
def do_GET(self):
713-
pass
720+
body = json.dumps(
721+
{"name": "test", "version": {"number": "0.0.0"}}
722+
).encode()
723+
self.send_response(200)
724+
self.send_header("Content-Type", "application/json")
725+
self.send_header("Content-Length", str(len(body)))
726+
self.end_headers()
727+
self.wfile.write(body)
714728

715729

716730
def test_default_schema(serve_http):

0 commit comments

Comments
 (0)