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

Update SQL Server JDBC driver to 10.2.0 #10898

Merged
merged 1 commit into from
Mar 1, 2022

Conversation

mosabua
Copy link
Member

@mosabua mosabua commented Feb 1, 2022

Description

Update the JDBC driver to the new major release version. Using the Java 11 version. A Java 17 one is also available if desired. This enables a simpler upgrade to the Java 17 version in the future when we update Trino to use 17.

See release notes for the new version for details. https://docs.microsoft.com/en-us/sql/connect/jdbc/release-notes-for-the-jdbc-driver?view=sql-server-ver15#94

General information

Is this change a fix, improvement, new feature, refactoring, or other?

Improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces (be specific)?

SQL Server connector only

How would you describe this change to a non-technical end user or system administrator?

Using the new JDBC driver enables us to switch to the Java 17 version soon. The old version does not have Java 17 support.

Related issues and pull requests

Documentation

( ) No documentation is needed.
(✅ ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(✅ ) Release notes entries required with the following suggested text:

## SQL Server connector

* Update JDBC driver to 10.2.0. Upgrade requires catalog configuration changes since the new version automatically enables encryption.  ({issue}`10898`)

Release notes should link to the updated section in the docs about the Connection security.

@cla-bot cla-bot bot added the cla-signed label Feb 1, 2022
@mosabua mosabua requested a review from hashhar February 1, 2022 22:31
@mosabua mosabua added the WIP label Feb 1, 2022
@mosabua
Copy link
Member Author

mosabua commented Feb 1, 2022

Waiting for results from CI run to see how it goes..

@mosabua mosabua removed the WIP label Feb 1, 2022
@mosabua mosabua added the WIP label Feb 2, 2022
@mosabua
Copy link
Member Author

mosabua commented Feb 2, 2022

Giving this a WIP for now to wait and see what CI results are

pom.xml Show resolved Hide resolved
@mosabua
Copy link
Member Author

mosabua commented Feb 2, 2022

sigh ... looks like there are test failures with this driver. We will have to investigate some more..

Copy link
Member

@ebyhr ebyhr left a comment

Choose a reason for hiding this comment

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

Specifying encrypt=false would resolve the test failure.

pom.xml Show resolved Hide resolved
@hashhar
Copy link
Member

hashhar commented Feb 2, 2022

The driver has breaking changes for TLS handling. encryption is enabled by default, and certificate verification is also enabled by default.

We should point users to the JDBC driver release notes to check them and adjust accordingly.
Maybe something like:

* Update JDBC driver to 10.2.0. There are some breaking changes in the driver - see https://docs.microsoft.com/en-us/sql/connect/jdbc/release-notes-for-the-jdbc-driver?view=sql-server-ver15#94 for details. ({issue}`10898`)

@findepi findepi changed the title Update SQL server JDBC driver to 10.2.0 for Java 11 Update SQL server JDBC driver to 10.2.0 Feb 2, 2022
@mosabua mosabua changed the title Update SQL server JDBC driver to 10.2.0 Update SQL Server JDBC driver to 10.2.0 Feb 2, 2022
@mosabua
Copy link
Member Author

mosabua commented Feb 2, 2022

Specifying encrypt=false would resolve the test failure.

Do you know where? I have to dig into this still. I assume in the connection URL in the test or so .. right?

@ebyhr
Copy link
Member

ebyhr commented Feb 3, 2022

@mosabua You can add withUrlParam("encrypt", "false"); to TestingSqlServer L114.

@mosabua mosabua force-pushed the ms-jdbc-17 branch 3 times, most recently from ebbcd27 to 7474057 Compare February 25, 2022 06:05
@mosabua mosabua removed the WIP label Feb 25, 2022
@mosabua
Copy link
Member Author

mosabua commented Feb 25, 2022

Should be good for review now @ebyhr and @hashhar

hashhar
hashhar previously approved these changes Feb 26, 2022
@hashhar hashhar dismissed their stale review February 26, 2022 06:07

Product tests are still failing

@hashhar
Copy link
Member

hashhar commented Feb 28, 2022

URL needs to be changed in

too.

@mosabua
Copy link
Member Author

mosabua commented Feb 28, 2022

Thanks for letting me know @hashhar . I haven't looked yet .. but have now fix this as well. Fingers crossed we get a passing build including tests.

@mosabua
Copy link
Member Author

mosabua commented Mar 1, 2022

From what I can tell the CI failures are not related to sqlserver anymore. Do I need to rebase or what can I do to get a green build overall @hashhar ? Or even a merge? Anything else I can do?

@hashhar
Copy link
Member

hashhar commented Mar 1, 2022

CI hit #11203

@hashhar hashhar merged commit 2c1344c into trinodb:master Mar 1, 2022
@hashhar hashhar mentioned this pull request Mar 1, 2022
@github-actions github-actions bot added this to the 372 milestone Mar 1, 2022
@mosabua mosabua deleted the ms-jdbc-17 branch March 1, 2022 15:39
@mosabua
Copy link
Member Author

mosabua commented Mar 1, 2022

Wow.. soo good. Thanks for all the help and the merge @hashhar and @ebyhr !

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