Fix: Fail early when database cluster does not respond #711
Conversation
|
""" WalkthroughThe changes update the connection logic to fail early when the database server is unresponsive, by raising the last encountered connection error if no valid server version is found. The changelog is updated to reflect this. Additionally, a new test is introduced to verify that connection errors are properly raised for invalid server addresses. The client connection documentation was simplified by removing an interactive example. The test suite registration for connection tests was moved to the integration test layer. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Connection
participant Server
Client->>Connection: connect()
loop For each server
Connection->>Server: request server version
alt Server responds with version
Connection->>Connection: store version if valid
else Server not available (ConnectionError)
Connection->>Connection: store last ConnectionError
else Invalid version (ValueError/InvalidVersion)
Connection->>Connection: ignore and continue
end
end
alt No valid server version found and ConnectionError occurred
Connection-->>Client: raise last ConnectionError
else Valid server version found
Connection-->>Client: proceed with connection
end
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (11)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| except (ValueError, ConnectionError): | ||
| except ConnectionError as ex: | ||
| last_connection_error = ex | ||
| continue |
There was a problem hiding this comment.
Won't this continue let you try all active servers? Do you want to stop at the first exception?
There was a problem hiding this comment.
Yes, like before, and no, like before. Do you think it should be done differently?
There was a problem hiding this comment.
I probably take the change log entry too literally.
There was a problem hiding this comment.
Sorry, so what's happening? does it retry all servers provided and then throw the last connection error? (shouldn't it throw the first connection error maybe?)
There was a problem hiding this comment.
If it would throw the first error, the whole logic of supporting multiple servers would be invalidated I think.
There was a problem hiding this comment.
ah sry got this wrong, not sure if throwing the first error would be better.
There was a problem hiding this comment.
another option could be to put all connection errors into an exception if all servers raise connection errors.
There was a problem hiding this comment.
If it would throw the first error, the whole logic of supporting multiple servers would be invalidated I think.
I share the same opinion that it would defeat the whole purpose of relevant routines, right.
There was a problem hiding this comment.
another option could be to put all connection errors into [a single] exception if all servers raise connection errors.
0046f03 implements this suggestion, thanks!
amotl
force-pushed
the
fail-on-connect
branch
3 times, most recently
from
fd7e0df to
7a285fe
Compare
| If no ``servers`` are given, the default one ``http://127.0.0.1:4200`` is used: | ||
|
|
||
| >>> connection = client.connect() | ||
| >>> connection.client._active_servers | ||
| ['http://127.0.0.1:4200'] | ||
| >>> connection.close() | ||
| If no ``servers`` are supplied to the ``connect`` method, the default address | ||
| ``http://127.0.0.1:4200`` is used. |
There was a problem hiding this comment.
There is no server at http://127.0.0.1:4200. This test case just didn't fail because connect() did not raise an exception up until now.
Now, there is no longer a way to validate connecting to the default address per doctest file, because there is no server listening at http://127.0.0.1:4200. Because the core information is still viable, all what's left is pure prose, rephrased a bit.
amotl
force-pushed
the
fail-on-connect
branch
3 times, most recently
from
306feb5 to
cb36d1b
Compare
src/crate/client/connection.py
Outdated
| if lowest is None and len(connection_errors) == server_count: | ||
| raise ConnectionError(str(connection_errors)) |
There was a problem hiding this comment.
str(connection_errors) uses an opaque way to serialize the list of exception instances into a string, but I think it is viable and does not cause too many surprises for callers that expect exception.message to be of str type. Do you have any objections or suggestions to do it differently, like using JSON instead? 1
Footnotes
-
In general,
raise ConnectionError(connection_errors)is also possible, but the caller then needs to handle the exception value as alisttype, which is semantically different on some occasions like"Server not available" in ex.exception.messagewould no longer beTrue, bearing potential breaking changes, at least for a few test cases of the test suite already. ↩
There was a problem hiding this comment.
[...] suggestions to do it differently, like using JSON instead?
The procedure now uses JSON instead of an opaque Python-serialized string per e818221.
For a single element, it looks like this now:
crate.client.exceptions.ConnectionError: ["Server not available, exception: HTTPConnectionPool(host='127.0.0.1', port=44209): Max retries exceeded with url: / (Caused by ReadTimeoutError(\"HTTPConnectionPool(host='127.0.0.1', port=44209): Read timed out. (read timeout=0.01)\"))"]-- https://github.com/crate/crate-python/actions/runs/14928213165/job/41937814085?pr=711#step:5:357
amotl
changed the title
The procedure will collect all `ConnectionError` instances and include them into the exception message of the final `ConnectionError`.
All `ConnectionError` instances that have been collected will be serialized into JSON now.
|
We just published a pre-release package including those updates per crate-2.1.0.dev0 and notified @shraik about it at crate/sqlalchemy-cratedb#218 (comment). |
| continue | ||
| if not lowest or version < lowest: | ||
| lowest = version | ||
| if connection_errors and len(connection_errors) == server_count: | ||
| raise ConnectionError(json.dumps(list(map(str, connection_errors)))) |
There was a problem hiding this comment.
why not just raise ConnectionError(error_list)?
There was a problem hiding this comment.
I've elaborated about it here, but I am also not sure, that's why I am also asking about your opinion.
... as suggested by warning message emitted by `python -m build`.
Problem
@shraik started using sqlalchemy-cratedb and reported that its behaviour deviates from other vendors by not failing on
engine.connect()when the database server is not available.We found this is not actually on the SQLAlchemy dialect, but on the DBAPI driver already, which exhibits the same behaviour.
Solution
The patch extends the
_lowest_server_versionmethod to re-raisethe lastaConnectionErrorwhen no connection can be made to any configured server nodeConnectionErrorwhen connecting to all server nodes fails, including all error messages.By doing it this way, we didn't need to submit a dummy SQL command like originally planned. We think it is much better this way because it does not pollute the server-side statement log.
Details
As an additional benefit, the software tests in
test_connection.pyare now actual integration tests./cc @mfussenegger, @seut, @surister