You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2021/05/05 13:08:12 UTC
[GitHub] [drill] martin-g opened a new pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
martin-g opened a new pull request #2217:
URL: https://github.com/apache/drill/pull/2217
# [DRILL-7911](https://issues.apache.org/jira/browse/DRILL-7911): Use TestContainers-MySQL instead of wix-embedded-mysql
## Description
Apache Drill build fails on Linux ARM64 because Wix Embedded MySQL does not support ARM64 and this library is no more maintained.
This PR replaces Wix Embedded MySQL with TestContainers-MySQL. In addition the PR sets Maven dependency management for JNA and Netty which support Linux ARM64.
## Documentation
There are no user visible changes. The changes are only in the tests.
## Testing
org.apache.drill.exec.store.jdbc.TestJdbcPluginWithMySQLIT has been updated to use TestContainers to run MySQL/MariaDB.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r628198591
##########
File path: pom.xml
##########
@@ -110,16 +110,18 @@
<codemodel.version>2.6</codemodel.version>
<joda.version>2.10.5</joda.version>
<javax.el.version>3.0.0</javax.el.version>
+ <jna.version>5.8.0</jna.version>
Review comment:
JNA is an alternative of JNI that is used by other libraries to communicate with native libraries.
There has been an improvement to JNA (and transitively to TestContainers) via https://github.com/testcontainers/testcontainers-java/issues/3610
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-843841717
The build has passed: 92759d96f258a1bcfd4eae33cf8f2410e406afbf
I've squashed the new commits.
I think it is ready to be merged!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
luocooong commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r634841835
##########
File path: pom.xml
##########
@@ -450,6 +452,8 @@
<exclude>**/*.prj</exclude>
<exclude>**/*.shp</exclude>
<exclude>**/*.dbf</exclude>
+ <!-- DRILL-7911 MySQL config overrides -->
Review comment:
@martin-g It's ready to merge. Could you please resolve the conflict and remove this comment (don't need it anymore)? thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r628199558
##########
File path: pom.xml
##########
@@ -110,16 +110,18 @@
<codemodel.version>2.6</codemodel.version>
<joda.version>2.10.5</joda.version>
<javax.el.version>3.0.0</javax.el.version>
+ <jna.version>5.8.0</jna.version>
<surefire.version>3.0.0-M4</surefire.version>
<commons.compress.version>1.20</commons.compress.version>
<hikari.version>3.4.2</hikari.version>
- <netty.version>4.1.59.Final</netty.version>
+ <netty.version>4.1.63.Final</netty.version>
Review comment:
I'll revert it. There is nothing aarch64 related in this version bump. The aarch64 improvements are only in netty-tcnative only.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-836711082
@luocooong
Are we good to merge this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-842406211
@martin-g Please squash all the commits (rebase on the mater).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-842871604
Squashed and rebased!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong merged pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
luocooong merged pull request #2217:
URL: https://github.com/apache/drill/pull/2217
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-842406211
@martin-g Please squash all the commits (rebase on the mater).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
luocooong commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r628186209
##########
File path: pom.xml
##########
@@ -1726,7 +1740,7 @@
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-tcnative</artifactId>
- <version>2.0.1.Final</version>
+ <version>2.0.39.Final</version>
Review comment:
Could you please explain the reasons for this upgrade?
##########
File path: pom.xml
##########
@@ -110,16 +110,18 @@
<codemodel.version>2.6</codemodel.version>
<joda.version>2.10.5</joda.version>
<javax.el.version>3.0.0</javax.el.version>
+ <jna.version>5.8.0</jna.version>
Review comment:
Could you please explain that the function of this library?
##########
File path: contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithMySQLIT.java
##########
@@ -51,51 +49,57 @@
@Category(JdbcStorageTest.class)
public class TestJdbcPluginWithMySQLIT extends ClusterTest {
- private static EmbeddedMysql mysqld;
+ private static JdbcDatabaseContainer<?> jdbcContainer;
@BeforeClass
public static void initMysql() throws Exception {
startCluster(ClusterFixture.builder(dirTestWatcher));
+ String osName = System.getProperty("os.name").toLowerCase();
String mysqlDBName = "drill_mysql_test";
- int mysqlPort = QueryTestUtil.getFreePortNumber(2215, 300);
- MysqldConfig config = MysqldConfig.aMysqldConfig(Version.v5_7_27)
- .withPort(mysqlPort)
- .withUser("mysqlUser", "mysqlPass")
- .withTimeZone(DateTimeZone.UTC.toTimeZone())
- .build();
+ DockerImageName imageName;
+ if (osName.startsWith("linux") && "aarch64".equals(System.getProperty("os.arch"))) {
+ imageName = DockerImageName.parse("mariadb:10.6.0").asCompatibleSubstituteFor("mysql");
+ } else {
+ imageName = DockerImageName.parse("mysql:5.7.27");
Review comment:
Could you please defined the version of db in the final variable?
##########
File path: pom.xml
##########
@@ -1161,6 +1165,16 @@
<!-- Managed Dependencies -->
<dependencyManagement>
<dependencies>
+ <dependency>
Review comment:
Can you add these library to the specified sub-module (Unless all the sub-moudle depend on it)?
##########
File path: pom.xml
##########
@@ -110,16 +110,18 @@
<codemodel.version>2.6</codemodel.version>
<joda.version>2.10.5</joda.version>
<javax.el.version>3.0.0</javax.el.version>
+ <jna.version>5.8.0</jna.version>
<surefire.version>3.0.0-M4</surefire.version>
<commons.compress.version>1.20</commons.compress.version>
<hikari.version>3.4.2</hikari.version>
- <netty.version>4.1.59.Final</netty.version>
+ <netty.version>4.1.63.Final</netty.version>
Review comment:
Netty is core of Drill RPC layer. So, Please separate this change into a new PR if the upgrade is necessary (for the aarch64).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r630116507
##########
File path: pom.xml
##########
@@ -110,16 +110,18 @@
<codemodel.version>2.6</codemodel.version>
<joda.version>2.10.5</joda.version>
<javax.el.version>3.0.0</javax.el.version>
+ <jna.version>5.8.0</jna.version>
Review comment:
Here is a build at Travis that fails due to the old JNA version in that branch: https://travis-ci.com/github/apache/drill/jobs/504592762
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-835288539
@martin-g Thanks for the revision. Because the ARM-based CI is not ready. At the same time, we are going to release the 1.19. So, Would you mind merging your PR (related the ARM) in next release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-838469176
> I recommend that merge this PR after the DRILL-7923.
As I said at https://github.com/apache/drill/pull/2220#issue-637610515 : `This PR depends on #2217, #2218 and #2219. The build will fail before those are merged.`
I.e. you could merge #2220 (DRILL-7923) first but the build at Travis will fail until this PR is merged too.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r628203535
##########
File path: pom.xml
##########
@@ -1726,7 +1740,7 @@
<dependency>
<groupId>io.netty</groupId>
<artifactId>netty-tcnative</artifactId>
- <version>2.0.1.Final</version>
+ <version>2.0.39.Final</version>
Review comment:
Netty-tcnative 2.0.31.Final has started providing aarch64 binaries for BoringSSL.
See https://github.com/netty/netty-tcnative/issues/552 and https://github.com/netty/netty-tcnative/pull/517
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r634964180
##########
File path: pom.xml
##########
@@ -450,6 +452,8 @@
<exclude>**/*.prj</exclude>
<exclude>**/*.shp</exclude>
<exclude>**/*.dbf</exclude>
+ <!-- DRILL-7911 MySQL config overrides -->
Review comment:
@luocooong Let's wait for a successful TravisCI build. It just failed due to some problem with TestContainers-Ryuk. I've updated TestContainers-Vault to 1.15.3.
This comment is documenting what is .cnf file. But I will remove it!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] luocooong commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
luocooong commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-836729205
@cgivre I recommend that merge this PR after the DRILL-7923.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on pull request #2217:
URL: https://github.com/apache/drill/pull/2217#issuecomment-835345660
Sure!
No problem!
On Sat, May 8, 2021, 13:49 luoc ***@***.***> wrote:
> @martin-g <https://github.com/martin-g> Thanks for the revision. Because
> the ARM-based CI is not ready. At the same time, we are going to release
> the 1.19. So, Would you mind merging your PR (related the ARM) in next
> release?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <https://github.com/apache/drill/pull/2217#issuecomment-835288539>, or
> unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABYUQVNCCPRWGKXXU5FUALTMUJMPANCNFSM44EWASXQ>
> .
>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] martin-g commented on a change in pull request #2217: DRILL-7911 Use TestContainers-MySQL instead of wix-embedded-mysql
Posted by GitBox <gi...@apache.org>.
martin-g commented on a change in pull request #2217:
URL: https://github.com/apache/drill/pull/2217#discussion_r628205134
##########
File path: contrib/storage-jdbc/src/test/java/org/apache/drill/exec/store/jdbc/TestJdbcPluginWithMySQLIT.java
##########
@@ -51,51 +49,57 @@
@Category(JdbcStorageTest.class)
public class TestJdbcPluginWithMySQLIT extends ClusterTest {
- private static EmbeddedMysql mysqld;
+ private static JdbcDatabaseContainer<?> jdbcContainer;
@BeforeClass
public static void initMysql() throws Exception {
startCluster(ClusterFixture.builder(dirTestWatcher));
+ String osName = System.getProperty("os.name").toLowerCase();
String mysqlDBName = "drill_mysql_test";
- int mysqlPort = QueryTestUtil.getFreePortNumber(2215, 300);
- MysqldConfig config = MysqldConfig.aMysqldConfig(Version.v5_7_27)
- .withPort(mysqlPort)
- .withUser("mysqlUser", "mysqlPass")
- .withTimeZone(DateTimeZone.UTC.toTimeZone())
- .build();
+ DockerImageName imageName;
+ if (osName.startsWith("linux") && "aarch64".equals(System.getProperty("os.arch"))) {
+ imageName = DockerImageName.parse("mariadb:10.6.0").asCompatibleSubstituteFor("mysql");
+ } else {
+ imageName = DockerImageName.parse("mysql:5.7.27");
Review comment:
Done!
##########
File path: pom.xml
##########
@@ -1161,6 +1165,16 @@
<!-- Managed Dependencies -->
<dependencyManagement>
<dependencies>
+ <dependency>
Review comment:
JNA comes as a transitive dependency of:
- org.kohsuke:libpam4j in drill-java-exec and drill-jdbc-all
- testcontainers/docker-java-api in all modules which use TestContainers (i.e. MySQL, Mongo, Splunk, Cassandra and Java Exec
I think defining it in the root pom is the best!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org