Add a MySQL JDBC driver extension #4067

machi1990 · 2019-09-17T17:48:39Z

machi1990 · 2019-09-17T18:00:52Z

...-mysql-deployment/src/main/java/io/quarkus/jdbc/mysql/deployment/MySQLDBJDBCReflections.java

+    }
+
+    @BuildStep
+    void registerExceptionsForReflection(BuildProducer<ReflectiveClassBuildItem> reflectiveClass) {


I pushed an upstream PR mysql/mysql-connector-j#43 to reduce some of these. The PR has been moved in the internal queue mysql/mysql-connector-j#43 (comment), hope to see it merged.

extensions/jdbc/pom.xml

integration-tests/jpa-mysqldb/README.md

integration-tests/jpa-mysqldb/pom.xml

Sanne

Hi @machi1990 that's awesome progress, thanks a lot!

I made some minor remarks in various comments - please have a look.

Most importantly, I tried to run it in native but couldn't correctly connect to the database. It's a bit late now so I didn't investigate yet - I will continue with that tomorrow, but could you check in the meantime that the integration test worked fine for you even in native mode?

Thanks!

Sanne · 2019-09-18T21:22:30Z

This seems to be the main problem:

Caused by: java.sql.SQLNonTransientConnectionException: Could not create connection to database server.
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:110)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:97)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:89)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:63)
	at com.mysql.cj.jdbc.ConnectionImpl.connectOneTryOnly(ConnectionImpl.java:1000)
	at com.mysql.cj.jdbc.ConnectionImpl.createNewIO(ConnectionImpl.java:817)
	at com.mysql.cj.jdbc.ConnectionImpl.<init>(ConnectionImpl.java:447)
	at com.mysql.cj.jdbc.ConnectionImpl.getInstance(ConnectionImpl.java:237)
	at com.mysql.cj.jdbc.NonRegisteringDriver.connect(NonRegisteringDriver.java:199)
	at io.agroal.pool.ConnectionFactory.createConnection(ConnectionFactory.java:200)
	at io.agroal.pool.ConnectionPool$CreateConnectionTask.call(ConnectionPool.java:357)
	at io.agroal.pool.ConnectionPool$CreateConnectionTask.call(ConnectionPool.java:346)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at io.agroal.pool.util.PriorityScheduledExecutor.beforeExecute(PriorityScheduledExecutor.java:64)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1146)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
	at com.oracle.svm.core.thread.JavaThreads.threadStartRoutine(JavaThreads.java:460)
	at com.oracle.svm.core.posix.thread.PosixJavaThreads.pthreadStartRoutine(PosixJavaThreads.java:193)
Caused by: java.lang.NullPointerException
	at java.util.Properties$LineReader.readLine(Properties.java:434)
	at java.util.Properties.load0(Properties.java:353)
	at java.util.Properties.load(Properties.java:341)
	at com.mysql.cj.util.TimeUtil.loadTimeZoneMappings(TimeUtil.java:226)
	at com.mysql.cj.util.TimeUtil.getCanonicalTimezone(TimeUtil.java:123)
	at com.mysql.cj.protocol.a.NativeProtocol.configureTimezone(NativeProtocol.java:2139)
	at com.mysql.cj.protocol.a.NativeProtocol.initServerSession(NativeProtocol.java:2163)
	at com.mysql.cj.jdbc.ConnectionImpl.initializePropsFromServer(ConnectionImpl.java:1301)
	at com.mysql.cj.jdbc.ConnectionImpl.connectOneTryOnly(ConnectionImpl.java:958)
	... 14 more

Also, less cricital but I had to run the mysql docker image manually as the wait conditions for docker-maven-plugin aren't working on Linux either.

integration-tests/jpa-mysqldb/README.md

machi1990 · 2019-09-19T05:55:46Z

Hi @machi1990 that's awesome progress, thanks a lot!

I made some minor remarks in various comments - please have a look.

@Sanne thanks for the review. I have taken into account your feedback.

Most importantly, I tried to run it in native but couldn't correctly connect to the database. It's a bit late now so I didn't investigate yet - I will continue with that tomorrow, but could you check in the meantime that the integration test worked fine for you even in native mode?

Thanks!

This seems to be the main problem:

Caused by: java.sql.SQLNonTransientConnectionException: Could not create connection to database server.
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:110)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:97)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:89)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:63)
	at com.mysql.cj.jdbc.ConnectionImpl.connectOneTryOnly(ConnectionImpl.java:1000)
	at com.mysql.cj.jdbc.ConnectionImpl.createNewIO(ConnectionImpl.java:817)
	at com.mysql.cj.jdbc.ConnectionImpl.<init>(ConnectionImpl.java:447)
	at com.mysql.cj.jdbc.ConnectionImpl.getInstance(ConnectionImpl.java:237)
	at com.mysql.cj.jdbc.NonRegisteringDriver.connect(NonRegisteringDriver.java:199)
	at io.agroal.pool.ConnectionFactory.createConnection(ConnectionFactory.java:200)
	at io.agroal.pool.ConnectionPool$CreateConnectionTask.call(ConnectionPool.java:357)
	at io.agroal.pool.ConnectionPool$CreateConnectionTask.call(ConnectionPool.java:346)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at io.agroal.pool.util.PriorityScheduledExecutor.beforeExecute(PriorityScheduledExecutor.java:64)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1146)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
	at com.oracle.svm.core.thread.JavaThreads.threadStartRoutine(JavaThreads.java:460)
	at com.oracle.svm.core.posix.thread.PosixJavaThreads.pthreadStartRoutine(PosixJavaThreads.java:193)
Caused by: java.lang.NullPointerException
	at java.util.Properties$LineReader.readLine(Properties.java:434)
	at java.util.Properties.load0(Properties.java:353)
	at java.util.Properties.load(Properties.java:341)
	at com.mysql.cj.util.TimeUtil.loadTimeZoneMappings(TimeUtil.java:226)
	at com.mysql.cj.util.TimeUtil.getCanonicalTimezone(TimeUtil.java:123)
	at com.mysql.cj.protocol.a.NativeProtocol.configureTimezone(NativeProtocol.java:2139)
	at com.mysql.cj.protocol.a.NativeProtocol.initServerSession(NativeProtocol.java:2163)
	at com.mysql.cj.jdbc.ConnectionImpl.initializePropsFromServer(ConnectionImpl.java:1301)
	at com.mysql.cj.jdbc.ConnectionImpl.connectOneTryOnly(ConnectionImpl.java:958)
	... 14 more

I was able to run the integration test natively with the timezone property set on my connection url that's why I missed this error. I'll take a look later today. Thanks for spotting this.

Also, less cricital but I had to run the mysql docker image manually as the wait conditions for docker-maven-plugin aren't working on Linux either.

Yeah, I had intermittent problems too and decided to run the docker image manually too (on a separate terminal) and do my tests.

machi1990 · 2019-09-19T11:41:57Z

This seems to be the main problem:

Caused by: java.sql.SQLNonTransientConnectionException: Could not create connection to database server.
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:110)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:97)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:89)
	at com.mysql.cj.jdbc.exceptions.SQLError.createSQLException(SQLError.java:63)
	at com.mysql.cj.jdbc.ConnectionImpl.connectOneTryOnly(ConnectionImpl.java:1000)
	at com.mysql.cj.jdbc.ConnectionImpl.createNewIO(ConnectionImpl.java:817)
	at com.mysql.cj.jdbc.ConnectionImpl.<init>(ConnectionImpl.java:447)
	at com.mysql.cj.jdbc.ConnectionImpl.getInstance(ConnectionImpl.java:237)
	at com.mysql.cj.jdbc.NonRegisteringDriver.connect(NonRegisteringDriver.java:199)
	at io.agroal.pool.ConnectionFactory.createConnection(ConnectionFactory.java:200)
	at io.agroal.pool.ConnectionPool$CreateConnectionTask.call(ConnectionPool.java:357)
	at io.agroal.pool.ConnectionPool$CreateConnectionTask.call(ConnectionPool.java:346)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at io.agroal.pool.util.PriorityScheduledExecutor.beforeExecute(PriorityScheduledExecutor.java:64)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1146)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:624)
	at java.lang.Thread.run(Thread.java:748)
	at com.oracle.svm.core.thread.JavaThreads.threadStartRoutine(JavaThreads.java:460)
	at com.oracle.svm.core.posix.thread.PosixJavaThreads.pthreadStartRoutine(PosixJavaThreads.java:193)
Caused by: java.lang.NullPointerException
	at java.util.Properties$LineReader.readLine(Properties.java:434)
	at java.util.Properties.load0(Properties.java:353)
	at java.util.Properties.load(Properties.java:341)
	at com.mysql.cj.util.TimeUtil.loadTimeZoneMappings(TimeUtil.java:226)
	at com.mysql.cj.util.TimeUtil.getCanonicalTimezone(TimeUtil.java:123)
	at com.mysql.cj.protocol.a.NativeProtocol.configureTimezone(NativeProtocol.java:2139)
	at com.mysql.cj.protocol.a.NativeProtocol.initServerSession(NativeProtocol.java:2163)
	at com.mysql.cj.jdbc.ConnectionImpl.initializePropsFromServer(ConnectionImpl.java:1301)
	at com.mysql.cj.jdbc.ConnectionImpl.connectOneTryOnly(ConnectionImpl.java:958)
	... 14 more

Also, less cricital but I had to run the mysql docker image manually as the wait conditions for docker-maven-plugin aren't working on Linux either.

@Sanne Seems like this resource /com/mysql/cj/util/TimeZoneMapping.properties was missing from the native image.

To solve this, I used substitution to read the resource during build time and substitute the method that required it after trying producing a SubstrateResourceBuildItem and SubstrateResourceBundleBuildItem to solve the issue without success. WDYT?

gsmet · 2019-09-19T11:45:51Z

How do they load the properties? I would have expected a SubstrateResourceBuildItem to work OK.

machi1990 · 2019-09-19T11:58:08Z

How do they load the properties? I would have expected a SubstrateResourceBuildItem to work OK.

With this piece of code

TimeUtil.class.getResourceAsStream("/com/mysql/cj/util/TimeZoneMapping.properties");

the file TimeZoneMapping.properties is in the package com.mysql.cj.util

Sanne · 2019-09-19T12:16:10Z

I guess there's a little performance benefit in the approach you took as you pre-parse the stream.
But is it worth it? Are there other reasons to not use a simple SubstrateResourceBuildItem ?

If you want to keep the current approach, maybe it;s worth doing one more step and also run the iteration on TimeZone.getAvailableIDs() on the properties before they are recorded?

machi1990 · 2019-09-19T12:46:17Z

I guess there's a little performance benefit in the approach you took as you pre-parse the stream.
But is it worth it? Are there other reasons to not use a simple SubstrateResourceBuildItem ?

SubstrateResourceBuildItem did not work, maybe I did something wrong. Also +1 with perf benefit.

If you want to keep the current approach, maybe it;s worth doing one more step and also run the iteration on TimeZone.getAvailableIDs() on the properties before they are recorded?

+1. I'll do this.

Sanne · 2019-09-19T13:12:00Z

hi @machi1990 , could you also rebase and test it on the rebased version please?

I'm afraid the tests need some additional work after recent merges to master, as the servlet API is no longer available by default to the JPAFunctionalityTestEndpoint

gsmet · 2019-09-19T13:31:05Z

If possible, I would avoid fiddling with substitutions if we can get SubstrateResourceBuildItem to work. I don't think the potential perf benefit is worth the maintenance burden. How did you register the resource? Maybe it's about the path not being right?

…

On Thu, Sep 19, 2019 at 3:12 PM Sanne Grinovero ***@***.***> wrote: hi @machi1990 <https://github.com/machi1990> , could you also rebase and test it on the rebased version please? I'm afraid the tests need some additional work after recent merges to master, as the servlet API is no longer available by default to the JPAFunctionalityTestEndpoint — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4067?email_source=notifications&email_token=AAJYOBILSDWISJS72LRC7DTQKN3CHA5CNFSM4IXTGXUKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD7DNGKA#issuecomment-533123880>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAJYOBJDQZC2XVIHEZPUCEDQKN3CHANCNFSM4IXTGXUA> .

machi1990 · 2019-09-19T14:38:25Z

If possible, I would avoid fiddling with substitutions if we can get SubstrateResourceBuildItem to work. I don't think the potential perf benefit is worth the maintenance burden. How did you register the resource? Maybe it's about the path not being right?

I tried producing a new SubstrateResourceBuildItem("/com/mysql/cj/util/TimeZoneMapping.properties") build item.

gsmet · 2019-09-19T14:49:19Z

I don't think the initial / would help. I'm also not sure if you need a .* at the beginning i.e. if the path is relative or absolute.

You can add -H:Log=registerResource to your native image build to have a bit more info about what's going on.

machi1990 · 2019-09-20T14:36:30Z

Hmm this test failure https://dev.azure.com/quarkus-ci/quarkus/_build/results?buildId=8111&view=logs

No LastRequestResponse on exception HttpRequestException: An error occurred while sending the request. System.Net.Http.HttpRequestException: An error occurred while sending the request. ---> System.Net.Http.CurlException: Couldn't connect to server
   at System.Net.Http.CurlHandler.ThrowIfCURLEError(CURLcode error)
   at System.Net.Http.CurlHandler.MultiAgent.FinishRequest(StrongToWeakReference`1 easyWrapper, CURLcode messageResult)
   --- End of inner exception stack trace ---
   at System.Net.Http.HttpClient.FinishSendAsyncBuffered(Task`1 sendTask, HttpRequestMessage request, CancellationTokenSource cts, Boolean disposeCts)
   at Microsoft.VisualStudio.Services.Common.TaskCancellationExtensions.EnforceCancellation[TResult](Task`1 task, CancellationToken cancellationToken, Func`1 makeMessage, String file, String member, Int32 line)
   at Microsoft.VisualStudio.Services.BlobStore.WebApi.DedupStoreHttpClient.<>c__DisplayClass56_0.<<GetRedirectResponseAsync>b__0>d.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at Microsoft.VisualStudio.Services.Content.Common.AsyncHttpRetryHelper`1.InvokeAsync(CancellationToken cancellationToken)
Information, Downloaded 44.4 MB out of 714.8 MB (6%).

seems unrelated. I forced pushed to re-launch the build.

gsmet

Hi @machi1990 ,

Very nice work as usual. I added a few questions and a few suggestions. Could you have a look?

As for CI, I don't know if we could support MySQL testing at least for JVM Linux. It would be simple if we don't already test MariaDB :).

As for the suggestion for native and initializing the class at runtime, I think it's worth a try. It really looks like a class that should be initialized at runtime and it should solve the threading issue. Or maybe not if we end up hitting a GraalVM bug but, in any case, it seems like the direction we should follow.

Feel free to ping me on Zulip if you want to chat about it!

gsmet · 2019-09-23T19:29:59Z

azure-pipelines.yml

+          modules:
+            - jpa-mysql
+          name: jpa_mysql
+          mysql: true


I think I would have made it part of the job below, which has only 2 tasks right now (jpa and jpa-postgresql).

There wouldn't be any port conflict so I think we are relatively safe.

.../main/java/io/quarkus/jdbc/mysql/runtime/graal/AbandonedConnectionCleanupThread_disable.java

ci-templates/native-build-steps.yaml

fixes quarkusio#2323

machi1990 · 2019-09-24T05:37:17Z

Hi @machi1990 ,

Very nice work as usual. I added a few questions and a few suggestions. Could you have a look?

As for CI, I don't know if we could support MySQL testing at least for JVM Linux. It would be simple if we don't already test MariaDB :).

Done.

As for the suggestion for native and initializing the class at runtime, I think it's worth a try. It really looks like a class that should be initialized at runtime and it should solve the threading issue. Or maybe not if we end up hitting a GraalVM bug but, in any case, it seems like the direction we should follow.

Feel free to ping me on Zulip if you want to chat about it!

Thanks Guillaume for your availability yesterday. Additional comments added for the AbandonedConnectionCleanupThread substitution.

gsmet · 2019-09-24T10:21:13Z

@Sanne I think we're all good now. As you requested changes, I let you check them and merge if you think it's OK.

Thanks @machi1990 , great work.

Sanne · 2019-09-24T12:15:21Z

Tested it all once more locally. Awesome, great job!

Sanne · 2019-09-24T12:16:27Z

@gsmet I see it's marked for milestone 0.24 rather than 0.23. Does it mean I should hold to merge?

Or feel free to merge.. thanks!

machi1990 · 2019-09-24T12:16:33Z

@gsmet @Sanne great. Thanks for your helpful tips and reviews on this one.

gsmet · 2019-09-24T13:02:44Z

master is 0.24.0 right now, we have a branch for 0.23 release. Things will be easier tomorrow hopefully.

Merging!

machi1990 requested review from gsmet and Sanne and removed request for gsmet September 17, 2019 17:48

machi1990 commented Sep 17, 2019

View reviewed changes

machi1990 force-pushed the jdbc-mysql-extension branch from 42b5098 to dd43f63 Compare September 17, 2019 18:26

machi1990 added the area/persistence label Sep 17, 2019

gsmet added this to the 0.24.0 milestone Sep 18, 2019

gsmet mentioned this pull request Sep 18, 2019

Quarkus Support for MySQL #2323

Closed

Sanne reviewed Sep 18, 2019

View reviewed changes

extensions/jdbc/pom.xml Outdated Show resolved Hide resolved

integration-tests/jpa-mysqldb/README.md Outdated Show resolved Hide resolved

integration-tests/jpa-mysqldb/README.md Outdated Show resolved Hide resolved

integration-tests/jpa-mysqldb/pom.xml Outdated Show resolved Hide resolved

Sanne requested changes Sep 18, 2019

View reviewed changes

Sanne reviewed Sep 18, 2019

View reviewed changes

integration-tests/jpa-mysqldb/README.md Outdated Show resolved Hide resolved

machi1990 force-pushed the jdbc-mysql-extension branch from dd43f63 to 4ad6b85 Compare September 19, 2019 05:41

machi1990 force-pushed the jdbc-mysql-extension branch from 4ad6b85 to a936d5f Compare September 19, 2019 09:01

machi1990 added the coding-in-progress label Sep 19, 2019

machi1990 force-pushed the jdbc-mysql-extension branch from a936d5f to 6516bdd Compare September 19, 2019 11:38

machi1990 requested a review from Sanne September 19, 2019 11:38

machi1990 force-pushed the jdbc-mysql-extension branch from 6516bdd to 965b47c Compare September 19, 2019 14:46

machi1990 removed the coding-in-progress label Sep 19, 2019

machi1990 force-pushed the jdbc-mysql-extension branch from 965b47c to 8282002 Compare September 19, 2019 15:49

machi1990 force-pushed the jdbc-mysql-extension branch 6 times, most recently from 01743ce to 1951e36 Compare September 20, 2019 14:35

machi1990 requested a review from gsmet September 20, 2019 16:57

machi1990 mentioned this pull request Sep 23, 2019

MariaDB JDBC driver update #4164

Merged

gsmet requested changes Sep 23, 2019

View reviewed changes

feat(jdbc-mysql): add support for MySQL

5fee229

fixes quarkusio#2323

machi1990 force-pushed the jdbc-mysql-extension branch from 1951e36 to 40d0a6e Compare September 23, 2019 21:35

ci: add mysql jdbc native test

e2697d0

machi1990 force-pushed the jdbc-mysql-extension branch from 40d0a6e to e2697d0 Compare September 23, 2019 22:03

gsmet approved these changes Sep 24, 2019

View reviewed changes

Sanne approved these changes Sep 24, 2019

View reviewed changes

gsmet merged commit 3a1d51b into quarkusio:master Sep 24, 2019

gsmet added the release/noteworthy-feature label Sep 24, 2019

machi1990 deleted the jdbc-mysql-extension branch September 24, 2019 13:06

gsmet added backport? and removed backport? labels Sep 24, 2019

gsmet modified the milestones: 0.24.0, 0.23.0 Sep 24, 2019

gsmet changed the title ~~add support for MySQL~~ Add a MySQL JDBC driver extension Sep 25, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a MySQL JDBC driver extension #4067

Add a MySQL JDBC driver extension #4067

machi1990 commented Sep 17, 2019

machi1990 Sep 17, 2019

Sanne left a comment

Sanne commented Sep 18, 2019

machi1990 commented Sep 19, 2019

machi1990 commented Sep 19, 2019

gsmet commented Sep 19, 2019

machi1990 commented Sep 19, 2019

Sanne commented Sep 19, 2019

machi1990 commented Sep 19, 2019

Sanne commented Sep 19, 2019

gsmet commented Sep 19, 2019 via email

machi1990 commented Sep 19, 2019

gsmet commented Sep 19, 2019

machi1990 commented Sep 20, 2019

gsmet left a comment

gsmet Sep 23, 2019

machi1990 commented Sep 24, 2019

gsmet commented Sep 24, 2019

Sanne commented Sep 24, 2019

Sanne commented Sep 24, 2019

machi1990 commented Sep 24, 2019

gsmet commented Sep 24, 2019

Add a MySQL JDBC driver extension #4067

Add a MySQL JDBC driver extension #4067

Conversation

machi1990 commented Sep 17, 2019

machi1990 Sep 17, 2019

Choose a reason for hiding this comment

Sanne left a comment

Choose a reason for hiding this comment

Sanne commented Sep 18, 2019

machi1990 commented Sep 19, 2019

machi1990 commented Sep 19, 2019

gsmet commented Sep 19, 2019

machi1990 commented Sep 19, 2019

Sanne commented Sep 19, 2019

machi1990 commented Sep 19, 2019

Sanne commented Sep 19, 2019

gsmet commented Sep 19, 2019 via email

machi1990 commented Sep 19, 2019

gsmet commented Sep 19, 2019

machi1990 commented Sep 20, 2019

gsmet left a comment

Choose a reason for hiding this comment

gsmet Sep 23, 2019

Choose a reason for hiding this comment

machi1990 commented Sep 24, 2019

gsmet commented Sep 24, 2019

Sanne commented Sep 24, 2019

Sanne commented Sep 24, 2019

machi1990 commented Sep 24, 2019

gsmet commented Sep 24, 2019