Allow to reuse few testing servers #6776

kokosing · 2021-01-30T22:21:56Z

Allow to reuse few testing servers

No plans to reuse containers in CI. This is only for development
purposes. Notice that it not always bring much value and some test do
not play well with reusing as they have side effects. Use it wisely on
your own responsibility.

Locally starting TestingMySqlServer and loading tpch data there takes
over a minute, with reusing it is barely noticeable.

Allow to reuse:

TestingSqlServer
TestingMySqlServer

skrzypo987

What is the motivation behind this change? You state that the gain is barely noticeable, yet this adds complexity.

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java

skrzypo987 · 2021-02-01T07:26:40Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

@@ -42,7 +46,7 @@ public TestingMySqlServer(boolean globalTransactionEnable)
    public TestingMySqlServer(String dockerImageName, boolean globalTransactionEnable)
    {
        container = createContainer(dockerImageName, globalTransactionEnable);
-        container.start();
+        closer.register(startOrReuse(container));


Why using closer for a single, fixed resource? Just adds complexity.

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java

kokosing · 2021-02-01T10:49:09Z

What is the motivation behind this change? You state that the gain is barely noticeable, yet this adds complexity.

@skrzypo987 Please see: #6776 (comment)

Locally starting TestingMySqlServer and loading tpch data there takes
over a minute, with reusing it is barely noticeable.

When you repeat tests frequently, then not waiting minute and a half to load tpch data each single tests executions is nice.

kokosing · 2021-02-01T11:01:16Z

@skrzypo987 AC, PTAL

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/MySqlQueryRunner.java

testing/trino-testing/src/main/java/io/trino/testing/containers/TestContainers.java

ssheikin · 2021-02-01T11:53:45Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java

@@ -56,7 +64,7 @@ public void execute(String sql)

    public void start()
    {
-        container.start();
+        closeable = startOrReuse(container);
        setUpDatabase();


It's not expected to set up database if you reuse container.

It is executed each time unfortunately. A room for improvement.

please double check that it does not fail on existing tables/indices/etc

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

ssheikin · 2021-02-01T12:03:00Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java


 public class TestingMySqlServer
-        extends MySQLContainer<TestingMySqlServer>


consider extracting common TestingReusableServer

I considered this and we are not there yet.

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

findepi · 2021-02-01T12:09:05Z

is this based on #6759 ?

kokosing · 2021-02-01T12:20:32Z

is this based on #6759 ?

No. It is separate.

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/MySqlQueryRunner.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

kokosing

@findepi @ssheikin PTALA, AC

kokosing · 2021-02-01T21:09:04Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java


 public class TestingMySqlServer
-        extends MySQLContainer<TestingMySqlServer>


I considered this and we are not there yet.

kokosing · 2021-02-01T21:17:57Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java

@@ -56,7 +64,7 @@ public void execute(String sql)

    public void start()
    {
-        container.start();
+        closeable = startOrReuse(container);
        setUpDatabase();


It is executed each time unfortunately. A room for improvement.

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/MySqlQueryRunner.java

kokosing · 2021-02-01T22:07:07Z

testing/trino-testing/src/main/java/io/trino/testing/containers/TestContainers.java

+    public static Closeable startOrReuse(GenericContainer<?> container)
+    {
+        checkState(
+                TestcontainersConfiguration.getInstance().environmentSupportsReuse() == TESTCONTAINERS_REUSE_ENABLE,


Please read the comment above:

// TESTCONTAINERS_REUSE_ENABLE is an environment variable defined in testcontainers library.

So I verify here that testcontainers behave as expected. They implemented this recently, so there is a risk of some movement here.

ssheikin · 2021-02-02T09:33:56Z

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

-    public String getJdbcUrl()
-    {
-        return format("jdbc:mysql://%s:%s?useSSL=false&allowPublicKeyRetrieval=true", getContainerIpAddress(), getMappedPort(MYSQL_PORT));
+        this.container = container;


Despite this.container is a reference, could you please move assignment to this to the very bottom of the constructor? That way it will be clear that everything above is preparation for the field.

this is the last place where this object is modified. Lines below do not prepare this object. What is worse, they could even container to be up and ready.

ssheikin · 2021-02-02T09:34:49Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java

@@ -56,7 +64,7 @@ public void execute(String sql)

    public void start()
    {
-        container.start();
+        closeable = startOrReuse(container);
        setUpDatabase();


please double check that it does not fail on existing tables/indices/etc

findepi

%

testing/trino-testing/src/main/java/io/trino/testing/QueryAssertions.java

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java

findepi · 2021-02-02T12:30:05Z

plugin/trino-sqlserver/src/test/java/io/trino/plugin/sqlserver/TestingSqlServer.java

@@ -30,6 +34,7 @@
    private final MSSQLServerContainer<?> container;
    private final boolean snapshotIsolationEnabled;
    private final String databaseName;
+    private Closeable reusableContainer = () -> {};


as part of unification we should remove the start method and initialize in ctor (as we do in others)

followup perhaps

This is something I don't like. Starting resources in constructor has short legs. Notice if you start a container in constructor and then exception is thrown there is no option to call close(). Maybe it is not a big deal, as we have failure anyway, but dangling container could influence other tests.

This is a design pattern we apply consistently except here. I am OK changing the principle, but as long as we did not change it, I do not see a compelling reason for TestingSqlServer to be different from others API-wise

Sounds good.

findepi · 2021-02-02T12:31:26Z

testing/trino-testing/src/main/java/io/trino/testing/containers/TestContainers.java

+
+        container.withReuse(TESTCONTAINERS_REUSE_ENABLE);
+        container.start();
+        if (supportsReuse) {


supportsReuse -> reuse
since you don't handle "i want to reuse, but that's not supported" case gracefully

findepi · 2021-02-02T12:32:12Z

testing/trino-testing/src/main/java/io/trino/testing/containers/TestContainers.java

+    private TestContainers() {}
+
+    /**
+     * You should not close the container directly if you want to reuse it.


Please make the first sentence of javadoc describe what the method does, or convert to non javadoc

findepi · 2021-02-02T12:32:40Z

testing/trino-tests/src/test/java/io/trino/tests/AbstractQueryAssertionsTest.java

-                        " <[varbinary]>\n" +
-                        "to be equal to:\n" +
-                        " <[varchar(6)]>");
+                .hasMessageContaining("[Output types] expected:<[var[char(6)]]> but was:<[var[binary]]>");


unrelated to the rest of Allow to reuse few testing servers commit?

It is related somehow. Adding testcontainers dependency updated assertion dependency and we got different message.

I thought we're pinning assetions (assertj?) version explicitly. Pulling in testcontainers shouldn't change anything here.

assertj didn't change:

kokosing@m16:~/presto$ diff before.txt after.txt 119,121c119,131 < [INFO] | \- org.jdbi:jdbi3-core:jar:3.17.0:compile < [INFO] | +- io.leangen.geantyref:geantyref:jar:1.3.7:compile < [INFO] | \- com.github.ben-manes.caffeine:caffeine:jar:2.8.0:compile --- > [INFO] | +- org.jdbi:jdbi3-core:jar:3.17.0:compile > [INFO] | | +- io.leangen.geantyref:geantyref:jar:1.3.7:compile > [INFO] | | \- com.github.ben-manes.caffeine:caffeine:jar:2.8.0:compile > [INFO] | \- org.testcontainers:testcontainers:jar:1.15.1:compile > [INFO] | +- junit:junit:jar:4.12:compile > [INFO] | | \- org.hamcrest:hamcrest-core:jar:1.3:compile > [INFO] | +- org.apache.commons:commons-compress:jar:1.20:compile > [INFO] | +- org.rnorth.duct-tape:duct-tape:jar:1.0.8:compile > [INFO] | +- org.rnorth.visible-assertions:visible-assertions:jar:2.1.2:compile > [INFO] | | \- net.java.dev.jna:jna:jar:5.5.0:compile > [INFO] | +- com.github.docker-java:docker-java-api:jar:3.2.7:compile > [INFO] | \- com.github.docker-java:docker-java-transport-zerodep:jar:3.2.7:compile > [INFO] | \- com.github.docker-java:docker-java-transport:jar:3.2.7:compile 190,191c200,201 < [INFO] Total time: 2.243 s < [INFO] Finished at: 2021-02-03T09:36:52+01:00 --- > [INFO] Total time: 1.993 s > [INFO] Finished at: 2021-02-03T09:34:17+01:00

Maybe the fact that org.hamcrest:hamcrest-core:jar:1.3:compile is on the classpath changes the assertion error message.

testing/trino-testing/src/main/java/io/trino/testing/containers/TestContainers.java

That plays well when environment is already existing.

This could leak undesired testcontainers API to tests. Also it makes it less customizable. In general inheritance should be avoid if possible. Only MySql and SqlServer are here changed.

No plans to reuse containers in CI. This is only for development purposes. Notice that it not always bring much value and some test do not play well with reusing as they have side effects. Use it wisely on your own responsibility. Locally starting TestingMySqlServer and loading tpch data there takes over a minute, with reusing it is barely noticeable. Allow to reuse: - TestingSqlServer - TestingMySqlServer

findepi · 2021-07-26T09:42:25Z

testing/trino-testing/src/main/java/io/trino/testing/containers/TestContainers.java

+    public static Closeable startOrReuse(GenericContainer<?> container)
+    {
+        boolean reuse = TestcontainersConfiguration.getInstance().environmentSupportsReuse();
+        checkState(reuse == TESTCONTAINERS_REUSE_ENABLE, "Unable to enable or disable container reuse");


Why we need both settings: the TESTCONTAINERS_REUSE_ENABLE env var & ~/.testcontainers.properties entry?
wouldn't it be possible to have this controlled with one toggle?

It is controlled with one toggle. The env var TESTCONTAINERS_REUSE_ENABLE. It is testcontainers that is changing the ~/.testcontainers.properties when we change TESTCONTAINERS_REUSE_ENABLE. Basically we verify if testcontainers behaviour didn't change.

are you saying that when i set env TESTCONTAINERS_REUSE_ENABLE=true, TC will automatically change my ~/.testcontainers.properties?

but then, when i unset TESTCONTAINERS_REUSE_ENABLE, it doesn't revert the file, so i need to manually clean it up

Not exactly.

I am saying that using TESTCONTAINERS_REUSE_ENABLE overrides memory representation of ~/.testcontainers.properties. This is testcontainers implementation detail that we check has not changed when the library is updated.

I think I know what is your problem. You changed ~/.testcontainers.properties globally (the thing that we were not planning to change when this code was added) and now you need to set TESTCONTAINERS_REUSE_ENABLE to make it work. Maybe we can relax this check to skip it when ! TESTCONTAINERS_REUSE_ENABLE.

Oh, so i roll back my changes in ~/.testcontainers.properties and now the TESTCONTAINERS_REUSE_ENABLE env var is all i needed to know about?
thanks, that's exactly what i need.

the TESTCONTAINERS_REUSE_ENABLE env var is all i needed to know

Yes. That was the plan to have a single and simple toggle.

cla-bot bot added the cla-signed label Jan 30, 2021

kokosing requested review from findepi, ssheikin and skrzypo987 January 30, 2021 22:22

kokosing force-pushed the origin/master/272_reuse branch from 5908171 to fbd70b9 Compare January 30, 2021 23:19

skrzypo987 reviewed Feb 1, 2021

View reviewed changes

kokosing force-pushed the origin/master/272_reuse branch from fbd70b9 to 57c6ec0 Compare February 1, 2021 11:01

skrzypo987 approved these changes Feb 1, 2021

View reviewed changes

plugin/trino-mysql/src/test/java/io/trino/plugin/mysql/TestingMySqlServer.java Outdated Show resolved Hide resolved

kokosing requested a review from losipiuk February 1, 2021 11:20

ssheikin reviewed Feb 1, 2021

View reviewed changes

kokosing mentioned this pull request Feb 1, 2021

Fix case insensitive predicate pushdown for MySQL, MemSQL and SQL Server #6753

Merged

kokosing force-pushed the origin/master/272_reuse branch from 57c6ec0 to 1eabe3e Compare February 1, 2021 13:59

findepi reviewed Feb 1, 2021

View reviewed changes

kokosing force-pushed the origin/master/272_reuse branch from 1eabe3e to a59bde3 Compare February 1, 2021 22:12

kokosing commented Feb 2, 2021

View reviewed changes

ssheikin reviewed Feb 2, 2021

View reviewed changes

ssheikin approved these changes Feb 2, 2021

View reviewed changes

findepi approved these changes Feb 2, 2021

View reviewed changes

findepi reviewed Feb 2, 2021

View reviewed changes

testing/trino-testing/src/main/java/io/trino/testing/containers/TestContainers.java Show resolved Hide resolved

kokosing force-pushed the origin/master/272_reuse branch 3 times, most recently from bc2207c to 8c65c9d Compare February 6, 2021 09:32

kokosing added 4 commits February 6, 2021 20:07

Do not use legacy config property name

036c763

Skip existing tables in copyTable

aab558f

That plays well when environment is already existing.

Make testing servers to not to extend containers

4fe08ad

This could leak undesired testcontainers API to tests. Also it makes it less customizable. In general inheritance should be avoid if possible. Only MySql and SqlServer are here changed.

kokosing force-pushed the origin/master/272_reuse branch from 8c65c9d to fbd1de0 Compare February 6, 2021 19:07

kokosing merged commit fbd1de0 into trinodb:master Feb 6, 2021

kokosing deleted the origin/master/272_reuse branch February 6, 2021 19:07

findepi reviewed Jul 26, 2021

View reviewed changes


		public class TestingMySqlServer
		extends MySQLContainer<TestingMySqlServer>

Allow to reuse few testing servers #6776

Allow to reuse few testing servers #6776

Conversation

kokosing commented Jan 30, 2021

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kokosing commented Feb 1, 2021

kokosing commented Feb 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi commented Feb 1, 2021

kokosing commented Feb 1, 2021

kokosing left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment