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

Check if url has https protocol scheme and add test #71

Merged
merged 2 commits into from
Nov 21, 2023

Conversation

andythsu
Copy link
Member

@andythsu andythsu commented Oct 7, 2023

No description provided.

Copy link
Member

@Chaho12 Chaho12 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I think there is an opportunity for further improvement per my comment.

@@ -62,7 +62,7 @@ public ClusterStats monitor(ProxyBackendConfiguration backend) {
jdbcUrl = String
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are getting smarter about setting the port based on the protocol, we should also use the backend URL to set the SSL parameter. Currently it is set in BackendStateConfiguration, so if you have a mixture of http and https backends it will necessarily be invalid for some of them.

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'm not familiar with SSL parameters. So I'm assuming SSL can be set depending on http or https?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep
https => SSL=true
http => SSL=false

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lets add that and then I think we should be good to merge this soon.

Comment on lines 33 to 37
private Connection con;
@Mock
private PreparedStatement stmt;
@Mock
private ResultSet rs;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit - use full names like connection, preparedStatement and resultSet.

<groupId>org.junit.jupiter</groupId>
<artifactId>junit-jupiter-api</artifactId>
<scope>test</scope>
</dependency>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removing this as this is a dup of line 42

Copy link
Contributor

@willmostly willmostly left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Copy link
Member

@mosabua mosabua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good.

@mosabua mosabua merged commit f734fa3 into trinodb:main Nov 21, 2023
2 checks passed
@andythsu andythsu deleted the improve_jdbc_healthcheck branch September 18, 2024 20:37
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