-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Report better message when no connection from driver #6872
Report better message when no connection from driver #6872
Conversation
@@ -73,7 +73,7 @@ public Connection openConnection(ConnectorSession session) | |||
JdbcIdentity identity = JdbcIdentity.from(session); | |||
Properties properties = getCredentialProperties(identity); | |||
Connection connection = driver.connect(connectionUrl.apply(identity), properties); | |||
checkState(connection != null, "Driver returned null connection"); | |||
checkState(connection != null, "Driver returned null connection, make sure the connection URL '%s' is valid for the driver %s", connectionUrl, driver); |
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 am not sure what to log here -- the connectionUrl
or connectionUrl.apply(identity)
.
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.
connectionUrl
is a Function
. So not a very loggable thing.
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.
You're right. I was too quick.
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 will chat with @kokosing why we need a function here.
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 think it is no longer used. And could be removed. I haven't found any usages of it.
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.
Thanks @kokosing for checking! will simplify
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 - this kind of message would help identify the cause of issue.
The connectionUrl never depends on identity.
edaf901
to
34a88dd
Compare
AC |
No description provided.