Use dockerized Cassandra in tests #2377

ebyhr · 2020-01-01T12:42:35Z

I tried org.testcontainers:cassandra at first, but it conflicted with the existing library and I couldn't find the benefit to use CassandraContainer instead of GenericContainer. I mean, even if we use CassandraContainer, the code will be almost the same in our test code.

2020-01-01T10:17:53.5606938Z +-io.prestosql:presto-cassandra:328-SNAPSHOT
2020-01-01T10:17:53.5607907Z   +-io.prestosql.cassandra:cassandra-driver:3.1.4-2
2020-01-01T10:17:53.5609386Z     +-io.netty:netty-handler:4.0.37.Final
2020-01-01T10:17:53.5610012Z and
2020-01-01T10:17:53.5612375Z +-io.prestosql:presto-cassandra:328-SNAPSHOT
2020-01-01T10:17:53.5653490Z   +-org.testcontainers:cassandra:1.12.3
2020-01-01T10:17:53.5654227Z     +-com.datastax.cassandra:cassandra-driver-core:3.7.1
2020-01-01T10:17:53.5655207Z       +-io.netty:netty-handler:4.0.56.Final
2020-01-01T10:17:53.5655323Z , 
2020-01-01T10:17:53.5655672Z Require upper bound dependencies error for io.dropwizard.metrics:metrics-core:3.1.2 paths to dependency are:
2020-01-01T10:17:53.5656098Z +-io.prestosql:presto-cassandra:328-SNAPSHOT
2020-01-01T10:17:53.5656330Z   +-io.prestosql.cassandra:cassandra-driver:3.1.4-2
2020-01-01T10:17:53.5767977Z     +-io.dropwizard.metrics:metrics-core:3.1.2
2020-01-01T10:17:53.5768427Z and
2020-01-01T10:17:53.5768962Z +-io.prestosql:presto-cassandra:328-SNAPSHOT
2020-01-01T10:17:53.5769501Z   +-org.testcontainers:cassandra:1.12.3
2020-01-01T10:17:53.5770041Z     +-com.datastax.cassandra:cassandra-driver-core:3.7.1
2020-01-01T10:17:53.5770587Z       +-io.dropwizard.metrics:metrics-core:3.2.2

findepi · 2020-01-01T13:43:14Z

I tried org.testcontainers:cassandra at first, but it conflicted with the existing library

@bsideup am I correct org.testcontainers:cassandra should have provided dependency on Cassandra driver?
https://github.com/testcontainers/testcontainers-java/blob/6007f7169265d8cf8b7294bdb9ce7d0c22f9e543/modules/cassandra/build.gradle#L5

even if we use CassandraContainer, the code will be almost the same in our test code

let's stay with generic container then, thanks @ebyhr for trying this out

findepi · 2020-01-01T13:44:36Z

presto-cassandra/pom.xml

@@ -160,8 +160,8 @@
        </dependency>

        <dependency>
-            <groupId>io.prestosql.cassandra</groupId>
-            <artifactId>cassandra-server</artifactId>


When this is merged, we should archive the https://github.com/prestosql/presto-cassandra-server repository.

Also remove this:
https://github.com/prestosql/presto/blob/c1e0d245787b503142a8b8dfc9733a34d0441017/pom.xml#L1190-L1200

findepi · 2020-01-01T13:49:41Z

presto-cassandra/src/test/java/io/prestosql/plugin/cassandra/CassandraQueryRunner.java

@@ -44,16 +44,16 @@ public static synchronized DistributedQueryRunner createCassandraQueryRunner()

        queryRunner.installPlugin(new CassandraPlugin());
        queryRunner.createCatalog("cassandra", "cassandra", ImmutableMap.of(
-                "cassandra.contact-points", EmbeddedCassandra.getHost(),
-                "cassandra.native-protocol-port", Integer.toString(EmbeddedCassandra.getPort()),
+                "cassandra.contact-points", CassandraServer.getHost(),


i am thinking it could make sense to convert CassandraServer to normal object, rather than having it a static instance.

-- are we benefiting from the reuse of single cassandra instance across test classes?

FWIW cassandra module runs tests single threaded today (https://github.com/prestosql/presto/blob/7aaa0f5fac1454b23e4bd2ac3a511d749bd00fbe/presto-cassandra/pom.xml#L17)

this means we don't need to worry about increased mem usage from multiple tests runs now

having CassandraServer as an object could allow us to run the tests in 2 threads (memory permitting)

how long does it take to start cassandra?

It takes about:

37s when the image does not exist in the environment (+data load)

17s when the image already exist in the environment (+data load)

Let me check the performance with normal object version.

Making CassandraServer non-static with 2 threads will extend the test time for a few minutes.
14m 34s → 16m 58s

I think it's worth it, since it brings more isolation.

17 minutes is still a decent run time for a test configuration.

(BTW Are there any test classes that could be easily merged?)

I think so too. Let me push the additional commit.

Currently, dockerized CassandraServer is called from 4 places.

TestCassandraDistributedQueries

TestCassandraIntegrationSmokeTest

TestCassandraConnector

TestCassandraTokenSplitManager

We could merge 3 & 4 technically, but I would keep the current implementation.

findepi · 2020-01-02T08:09:49Z

...o-cassandra/src/test/java/io/prestosql/plugin/cassandra/TestCassandraDistributedQueries.java

@@ -32,7 +32,7 @@
    protected QueryRunner createQueryRunner()
            throws Exception
    {
-        return CassandraQueryRunner.createCassandraQueryRunner();
+        return new CassandraQueryRunner().createCassandraQueryRunner();


You want to retain ref to CassandraServer so that you close the docker container in @AfterClass

it may be simpler to write if CassandraQueryRunner remains a utility class and simply takes the server as a param:

protected QueryRunner createQueryRunner() { this.server = new CassandraServer(); return CassandraQueryRunner.createCassandraQueryRunner(server); } @AfterClass(alwaysRun=true) public void tearDown() { this.server.close(); }

findepi · 2020-01-02T08:12:28Z

...to-cassandra/src/test/java/io/prestosql/plugin/cassandra/TestCassandraTokenSplitManager.java

    private CassandraSession session;
    private CassandraTokenSplitManager splitManager;

    @BeforeClass
    public void setUp()
            throws Exception
    {
-        CassandraServer.start();
-        session = CassandraServer.getSession();
+        cassandraServer = new CassandraServer();


Add @AfterClass(alwaysRun=true) to clean this up

ebyhr · 2020-01-02T09:22:32Z

Updated:

Remain static CassandraQueryRunner
Add Closeable to CassandraServer
Add tearDown to each tests

ebyhr · 2020-01-02T10:24:11Z

I'm checking the CI failure.

findepi

Thanks!

findepi · 2020-01-02T15:14:26Z

presto-cassandra/pom.xml

@@ -160,8 +160,8 @@
        </dependency>

        <dependency>
-            <groupId>io.prestosql.cassandra</groupId>
-            <artifactId>cassandra-server</artifactId>


Also remove this:
https://github.com/prestosql/presto/blob/c1e0d245787b503142a8b8dfc9733a34d0441017/pom.xml#L1190-L1200

findepi · 2020-01-02T15:16:07Z

presto-cassandra/src/test/java/io/prestosql/plugin/cassandra/CassandraServer.java

@@ -70,18 +68,16 @@ public static synchronized void start()

        log.info("Starting cassandra...");

-        System.setProperty("cassandra.config", "file:" + prepareCassandraYaml());
-        System.setProperty("cassandra-foreground", "true");
-        System.setProperty("cassandra.native.epoll.enabled", "false");


Was cassandra-foreground and cassandra.native.epoll.enabled something important?

Yes, we couldn't run tests without these properties on EmbeddedCassandra.

findepi · 2020-01-02T15:18:48Z

presto-cassandra/src/test/java/io/prestosql/plugin/cassandra/CassandraQueryRunner.java

-            }
-            tpchLoaded = true;
+        createKeyspace(server.getSession(), "tpch");
+        List<TpchTable<?>> tables = TpchTable.getTables();


We can probably save some time by limiting this to only the tables we need (some test classes need 1-2 tables). This is how we do for eg Postgres.

(no change requested; might be a follow-up)

findepi · 2020-01-02T15:22:12Z

presto-cassandra/src/test/java/io/prestosql/plugin/cassandra/CassandraTestingUtils.java

@@ -132,7 +132,7 @@ public static void createTableClusteringKeysInequality(CassandraSession session,

    public static void insertIntoTableClusteringKeysInequality(CassandraSession session, SchemaTableName table, Date date, int rowsCount)
    {
-        for (Integer rowNumber = 1; rowNumber <= rowsCount; rowNumber++) {


findepi · 2020-01-02T15:25:07Z

...o-cassandra/src/test/java/io/prestosql/plugin/cassandra/TestCassandraDistributedQueries.java


 import static io.prestosql.spi.type.VarcharType.VARCHAR;
 import static io.prestosql.testing.MaterializedResult.resultBuilder;
 import static io.prestosql.testing.assertions.Assert.assertEquals;

-//Integrations tests fail when parallel, due to a bug or configuration error in the embedded
-//cassandra instance. This problem results in either a hang in Thrift calls or broken sockets.


Let's see if this comment is still applicable.

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]> Review comments

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

…\n\nCherry-pick of trinodb/trino#2377 \n\nCo-authored-by: Yuya Ebihara <[email protected]>

… \n\n Cherry-pick of trinodb/trino#2377 \n\n Co-authored-by: Yuya Ebihara <[email protected]>

\n\n snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

cla-bot bot added the cla-signed label Jan 1, 2020

findepi reviewed Jan 1, 2020

View reviewed changes

ebyhr force-pushed the testcontainers-cassandra branch from 80450d4 to b18fff0 Compare January 2, 2020 04:34

findepi reviewed Jan 2, 2020

View reviewed changes

ebyhr force-pushed the testcontainers-cassandra branch from b18fff0 to 8d65d7f Compare January 2, 2020 09:18

ebyhr force-pushed the testcontainers-cassandra branch from 8d65d7f to 2c9eb36 Compare January 2, 2020 12:21

ebyhr requested a review from findepi January 2, 2020 13:23

findepi approved these changes Jan 2, 2020

View reviewed changes

findepi added maintenance Project maintenance task test labels Jan 2, 2020

ebyhr added 4 commits January 5, 2020 12:04

Use dockerized Cassandra in tests

1c37204

Make CassandraServer non-static

8baacf7

Run Cassandra tests with two threads

2738fef

Fix syntax warning in Cassandra test

f79bb44

ebyhr force-pushed the testcontainers-cassandra branch from 2c9eb36 to f79bb44 Compare January 5, 2020 03:04

ebyhr merged commit 2265041 into trinodb:master Jan 5, 2020

ebyhr deleted the testcontainers-cassandra branch January 5, 2020 08:37

ebyhr mentioned this pull request Jan 6, 2020

Load only required tables in CassandraQueryRunner #2413

Merged

nishithakbhaskaran mentioned this pull request Nov 25, 2024

snakeyaml cve fix prestodb/presto#24099

Merged

6 tasks

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 26, 2024

Fix snakeyaml CVEs

720d33f

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]> Review comments

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 26, 2024

Fix snakeyaml CVEs

229bdf6

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 26, 2024

Fix snakeyaml CVEs

0842929

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 28, 2024

Fix snakeyaml CVEs

b07d6d9

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 28, 2024

Fix snakeyaml CVEs

d677424

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 28, 2024

Fix snakeyaml CVEs

e360f76

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 28, 2024

Fix snakeyaml CVEs \n\nsnakeyaml 1.x version causing mend scan CVEs. …

57d15b5

…\n\nCherry-pick of trinodb/trino#2377 \n\nCo-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 28, 2024

Fix snakeyaml CVEs

e7f0e93

\n\n snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 28, 2024

Fix snakeyaml CVEs

568312e

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Nov 28, 2024

Fix snakeyaml CVEs

fcfb964

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

nishithakbhaskaran added a commit to nishithakbhaskaran/presto-oss-fix that referenced this pull request Dec 3, 2024

Fix snakeyaml CVEs

fc8ccd8

snakeyaml 1.x version causing mend scan CVEs. Cherry-pick of trinodb/trino#2377 Co-authored-by: Yuya Ebihara <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use dockerized Cassandra in tests #2377

Use dockerized Cassandra in tests #2377

ebyhr commented Jan 1, 2020

findepi commented Jan 1, 2020

findepi Jan 1, 2020

findepi Jan 2, 2020

findepi Jan 1, 2020

findepi Jan 1, 2020

ebyhr Jan 1, 2020 •

edited

Loading

ebyhr Jan 1, 2020

findepi Jan 1, 2020 •

edited

Loading

ebyhr Jan 2, 2020

findepi Jan 2, 2020

findepi Jan 2, 2020

ebyhr commented Jan 2, 2020

ebyhr commented Jan 2, 2020

findepi left a comment

findepi Jan 2, 2020

findepi Jan 2, 2020

ebyhr Jan 5, 2020

findepi Jan 2, 2020

findepi Jan 2, 2020

findepi Jan 2, 2020

Use dockerized Cassandra in tests #2377

Use dockerized Cassandra in tests #2377

Conversation

ebyhr commented Jan 1, 2020

findepi commented Jan 1, 2020

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

ebyhr Jan 1, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

findepi Jan 1, 2020 • edited Loading

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

ebyhr commented Jan 2, 2020

ebyhr commented Jan 2, 2020

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

ebyhr Jan 1, 2020 •

edited

Loading

findepi Jan 1, 2020 •

edited

Loading