-
Notifications
You must be signed in to change notification settings - Fork 31
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 our terms of service and privacy 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?
Changes from all commits
1262fe6
b74ce4b
d22fde9
51ead0a
891fc3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,8 +18,10 @@ | |
# However, if you have executed another commercial license agreement | ||
# with Crate these terms will supersede the license and you may use the | ||
# software solely pursuant to the terms of the relevant commercial agreement. | ||
import json | ||
|
||
from verlib2 import Version | ||
from verlib2.packaging.version import InvalidVersion | ||
|
||
from .blob import BlobContainer | ||
from .cursor import Cursor | ||
|
@@ -197,14 +199,21 @@ def get_blob_container(self, container_name): | |
|
||
def _lowest_server_version(self): | ||
lowest = None | ||
server_count = len(self.client.active_servers) | ||
connection_errors = [] | ||
for server in self.client.active_servers: | ||
try: | ||
_, _, version = self.client.server_infos(server) | ||
version = Version(version) | ||
except (ValueError, ConnectionError): | ||
except ConnectionError as ex: | ||
connection_errors.append(ex) | ||
continue | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Won't this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, like before, and no, like before. Do you think it should be done differently? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I probably take the change log entry too literally. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ah sry got this wrong, not sure if throwing the first error would be better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I share the same opinion that it would defeat the whole purpose of relevant routines, right. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
0046f03 implements this suggestion, thanks! |
||
except (ValueError, InvalidVersion): | ||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why not just There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've elaborated about it here, but I am also not sure, that's why I am also asking about your opinion. |
||
return lowest or Version("0.0.0") | ||
|
||
def __repr__(self): | ||
|
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 at
http://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 at
http://127.0.0.1:4200
. Because the core information is still viable, all what's left is pure prose, rephrased a bit.