Skip to content
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

Conversation

findepi
Copy link
Member

@findepi findepi commented Feb 11, 2021

No description provided.

@@ -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);
Copy link
Member Author

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).

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member Author

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

Copy link
Member

@ppalucha ppalucha left a 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.

@findepi findepi force-pushed the findepi/report-better-message-when-no-connection-from-driver-85c2f7 branch from edaf901 to 34a88dd Compare February 19, 2021 13:05
@findepi
Copy link
Member Author

findepi commented Feb 19, 2021

AC

@findepi
Copy link
Member Author

findepi commented Feb 21, 2021

CI #6223 and probably #6799

@findepi findepi merged commit 4ad6f7e into trinodb:master Feb 22, 2021
@findepi findepi deleted the findepi/report-better-message-when-no-connection-from-driver-85c2f7 branch February 22, 2021 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants