- Notifications
You must be signed in to change notification settings - Fork31
Fix: Fail early when database cluster does not respond#711
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Uh oh!
There was an error while loading.Please reload this page.
Conversation
coderabbitaibot commentedMay 6, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
""" 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 endPoem
📜 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 withCodeRabbit:
SupportNeed help? Create a ticket on oursupport 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 ( |
Uh oh!
There was an error while loading.Please reload this page.
fd7e0df to7a285feCompare| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
There is no server athttp://127.0.0.1:4200. This test case just didn't fail becauseconnect() 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 athttp://127.0.0.1:4200. Because the core information is still viable, all what's left is pure prose, rephrased a bit.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Lgtm
306feb5 tocb36d1bComparesrc/crate/client/connection.py Outdated
| iflowestisNoneandlen(connection_errors)==server_count: | ||
| raiseConnectionError(str(connection_errors)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 expectexception.message to be ofstr 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
[...] suggestions to do it differently, like using JSON instead?
The procedure now uses JSON instead of an opaque Python-serialized string pere818221.
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
The procedure will collect all `ConnectionError` instances and includethem into the exception message of the final `ConnectionError`.
All `ConnectionError` instances that have been collected will beserialized into JSON now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
amotl commentedMay 9, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
We just published a pre-release package including those updates percrate-2.1.0.dev0 and notified@shraik about it atcrate/sqlalchemy-cratedb#218 (comment). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@amotl Thank you! Is there a way to test that we don't fail the connection attempt, but only if only all servers provided fail?
Can we also test the exception when multiple nodes fail?
| ifnotlowestorversion<lowest: | ||
| lowest=version | ||
| ifconnection_errorsandlen(connection_errors)==server_count: | ||
| raiseConnectionError(json.dumps(list(map(str,connection_errors)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
why not justraise ConnectionError(error_list)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I've elaborated about ithere, 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`.
Uh oh!
There was an error while loading.Please reload this page.
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