Update Cassandra driver to 4.x by emerkle826 · Pull Request #2830 · testcontainers/testcontainers-java

emerkle826 · 2020-06-02T20:19:33Z

This PR updates the Cassandra driver to the newer 4.x version. It should address #2768.

rnorth · 2020-07-04T11:05:39Z

I'm a bit uncomfortable with this causing a breaking change in our APIs and transitive dependencies, but I'm not sure I can see any other way around it (short of adding an entire new class that uses the Cassandra 4.x drivers).

Will post a suggestion as a comment, though.

rnorth · 2020-07-04T11:17:22Z


-    public static Cluster getCluster(ContainerState containerState, boolean enableJmxReporting) {
-        final Cluster.Builder builder = Cluster.builder()
-            .addContactPoint(containerState.getHost())
-            .withPort(containerState.getMappedPort(CQL_PORT));
-        if (!enableJmxReporting) {
-            builder.withoutJMXReporting();
-        }
+    public static CqlSession getSession(ContainerState containerState) {
+        final CqlSessionBuilder builder = CqlSession.builder()
+            .addContactPoint(new InetSocketAddress(containerState.getHost(), containerState.getMappedPort(CQL_PORT)))
+            .withLocalDatacenter(DATACENTER);
        return builder.build();
    }


Returning a third-party class from our convenience method is what's now given us this breaking change headache. It's a pattern we're trying to avoid with new modules for this exact reason.

Could we consider eliminating com.datastax.* types from the API of this class?

e.g.:

delete the old getCluster() method as you've done

do not add a getSession method

add a new getContactPoint() method that simply returns an InetSocketAddress, and a getDataCenter() method that returns the DATACENTER constant

It would be slightly more work for users, e.g. they would have to do:

final CqlSessionBuilder builder = CqlSession.builder() .addContactPoint(myCassandraContainer.getContactPoint()) .withLocalDatacenter(myCassandraContainer.getDatacenter()); return builder.build();

but our public API from this class would just consist of JDK types, and we'd never have to make breaking changes again should (for example) the cassandra driver change classes/package names in v5.

If we're going to have to have a breaking change here, why not do it in a way that avoids future breaking changes?

BTW you'll notice I'm suggesting this be instance methods, not static methods - I am really quite surprised that these were ever static methods.

@rnorth Thank you for the review and the comments!
I see your point about Testcontainers concerns and I think you are right. I'm a bit tied up with a few other things at the moment, but I will revisit this as soon as I can and try to implement the changes you suggest. I'll also try to update the Cassandra Testcontainer documentation if necessary.

emerkle826 · 2020-07-31T21:04:14Z

I'm a bit uncomfortable with this causing a breaking change in our APIs and transitive dependencies, but I'm not sure I can see any other way around it (short of adding an entire new class that uses the Cassandra 4.x drivers).

Will post a suggestion as a comment, though.

@rnorth I've taken a second stab at this. Instead of removing the existing Driver 3.x API, I've chosen to deprecate the methods that return its Cluster object, while adding the new methods you suggested. This prevents introducing a breaking change for now, and provides a tiny convenience for using the container with Driver 4.x. I also updated the docs to demonstrate the new way of using it with the different driver versions.

However, I have left CassandraDatabaseDelegate and CassandraQueryWaitStrategy alone for now as I'm not sure the best solution to make either of those Driver agnostic. A solution could be to deprecate/remove the delegate and wait strategy, and maybe add more documentation of how to do it with each of the drivers. Or, I could duplicate everything you have currently and provide Driver 4 versions of the same classes. But I believe that is not really in the interests of Testcontainers, and probably creates more confusion that it does good.

tomekl007 · 2020-08-04T08:05:17Z

+            cassandraContainer.start();
+            // Trying to build a CqlSession will fail if a Contact Point is specified,
+            // but Local Datacenter is omitted.
+            CqlSession.builder().addContactPoint(cassandraContainer.getContactPoint()).build();


Maybe instead of @Test(expected = IllegalStateException.class), we could do a more detailed verification by catching an exception and validating that the exception message contains contact-points string? Or use AssertJ assertThatThrownBy

I've changed the test to verify part of the exception message here: https://github.com/testcontainers/testcontainers-java/pull/2830/files#diff-f060cde9ce304b8f1959fc12ce03a851R131-R132

tomekl007 · 2020-08-04T08:07:34Z

+     * @return The configured local Datacenter name.
+     * @see #withLocalDatacenter(java.lang.String)
+     */
+    public String getLocalDatacenter() {


nice that you added it, it was missing before.

stale · 2021-01-24T03:00:56Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If you believe this is a mistake, please reply to this comment to keep it open. If there isn't one already, a PR to fix or at least reproduce the problem in a test case will always help us get back on track to tackle this.

michaelyaakoby · 2021-01-24T06:50:32Z

I'm using the changes from this pull-request for many months already so we can test using latest cassandra driver.
Unless testcontainers already added this support, it would be strange if this PR is closed.

Maybe I or others can help with this?

pluttrell · 2021-02-07T08:01:50Z

@rnorth Soft ping. Wondering what it would take to get this to the finish line? I'd love to start using it.

bsideup · 2021-02-07T20:04:33Z

    private String configLocation;
    private String initScriptPath;
    private boolean enableJmxReporting;
+    private String localDatacenter;


this variable seems to be write-only. It does not end up in container's configuration (e.g. environment variable). Let's either make it affect the container or remove.

bsideup · 2021-02-07T20:04:39Z

    public static final String IMAGE = "cassandra";

    public static final Integer CQL_PORT = 9042;
+    public static final String DEFAULT_LOCAL_DATACENTER = "datacenter1";


Suggested change

public static final String DEFAULT_LOCAL_DATACENTER = "datacenter1";

private static final String DEFAULT_LOCAL_DATACENTER = "datacenter1";

bsideup · 2021-02-07T20:04:56Z


    /**
-     * Get configured Cluster
+     * Get configured Session


Why?

Suggested change

* Get configured Session

* Get configured Cluster

There is no more Cluster vs Session distinction in driver4. Your only entry point is the session, now called CqlSession. The word "cluster" becomes meaningless.

this only applies to v4, while in v3 (the currently used types that we're deprecating) it remains Cluster.

Whoops you are right, my bad.

Yes, one of my first commits for this changed getCluster() to getSession(), but that was reverted. I just forgot to revert the comment change.

bsideup · 2021-02-07T20:05:43Z

        try {
-            return CassandraContainer.getCluster(container)
-                    .newSession();
+            return CassandraContainer.getCluster(container).newSession();


let's revert this non-relevant change to reduce the changeset

bsideup · 2021-02-07T20:05:59Z


    private static final String TEST_CLUSTER_NAME_IN_CONF = "Test Cluster Integration Test";

+    private static final String BAISC_QUERY = "SELECT release_version FROM system.local";


typo, BAISC_QUERY -> BASIC_QUERY

bsideup · 2021-02-07T20:06:46Z

+    }
+
+    @Test
+    public void testMissingLocalDatacenter() {


this seems to be testing the driver, not the container. Let's remove it

kiview · 2022-07-20T14:54:22Z

+dependencies {
+    testImplementation 'org.testcontainers:cassandra'
+    testImplementation 'junit:junit:4.13.2'
+//    testRuntimeOnly 'com.datastax.cassandra:cassandra-driver-core:3.10.0'


Suggested change

// testRuntimeOnly 'com.datastax.cassandra:cassandra-driver-core:3.10.0'

Not needed, or?

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>

kiview · 2022-08-04T12:38:26Z

Merged, thank you @emerkle826 and everyone collaborating on this 🙂

nikeee · 2022-08-04T14:32:44Z

Thank you!

akhaku · 2022-09-07T23:06:07Z

Many thanks! Looking forward to picking this up after the next release

With testcontainers#2830 we added support for 4.x of the Cassandra driver. It was done in a way to allow a user to use either the 3.x or 4.x driver while excluding the other one. However if using 4.x and excluding 3.x, GenericContainer#canBeReused throws an exception during reflection since the Cluster class returned by CassandraContainer#getCluster is missing. This PR works around that issue by catching and ignoring the thrown NoClassDefFoundError.

With testcontainers#2830 we added some environment variable setup to the constructor in CassandraContainer. This causes problems in scenarios where users customize the environment themselves or customize the values in cassandra.yaml, so move this init to a protected method to facilitate overriding.

…5990) With #2830 we added support for 4.x of the Cassandra driver. It was done in a way to allow a user to use either the 3.x or 4.x driver while excluding the other one. However if using 4.x and excluding 3.x, GenericContainer#canBeReused throws an exception during reflection since the Cluster class returned by CassandraContainer#getCluster is missing. This PR works around that issue by catching and ignoring the thrown NoClassDefFoundError.

emerkle826 requested review from bsideup, kiview and rnorth as code owners June 2, 2020 20:19

emerkle826 force-pushed the cassandra-driver-upgrade branch from 8b4144d to 71b86b5 Compare June 4, 2020 15:38

rnorth requested changes Jul 4, 2020

View reviewed changes

rnorth added the type/breaking-api-change Public APIs are being changed label Jul 5, 2020

emerkle826 force-pushed the cassandra-driver-upgrade branch from 71b86b5 to 8803294 Compare July 31, 2020 20:53

tomekl007 reviewed Aug 4, 2020

View reviewed changes

adutra mentioned this pull request Aug 19, 2020

Upgrade Cassandra driver to 4.x #2768

Closed

emerkle826 mentioned this pull request Sep 14, 2020

Add Cassandra driver Actuator metrics spring-projects/spring-boot#23008

Closed

bsideup mentioned this pull request Sep 25, 2020

Add support for java-driver-core #3267

Closed

stale Bot added the stale label Jan 24, 2021

stale Bot removed the stale label Jan 24, 2021

bsideup reviewed Feb 7, 2021

View reviewed changes

emerkle826 added 3 commits February 8, 2021 08:47

Update Cassandra driver to 4.x

3851d25

Deprecate Driver 3.x methods and remove some driver coupling

503133c

improve verification of test for missing Datacenter

fe6b846

emerkle826 force-pushed the cassandra-driver-upgrade branch from cccd50e to 343f937 Compare February 8, 2021 14:55

address review comments

728dcdd

emerkle826 force-pushed the cassandra-driver-upgrade branch from 343f937 to 728dcdd Compare February 8, 2021 14:59

Merge branch 'master' into cassandra-driver-upgrade

b4b5523

emerkle826 requested a review from a team July 13, 2022 23:03

github-actions Bot added modules/cassandra type/docs labels Jul 13, 2022

eddumelendez added 2 commits July 13, 2022 18:13

Read/Write CASSANDRA_DC env

b3a3ac4

Fix message

64bd5f7

eddumelendez added type/enhancement and removed type/breaking-api-change Public APIs are being changed labels Jul 14, 2022

eddumelendez added this to the next milestone Jul 14, 2022

Use code snippets from examples

e7d5f9a

kiview reviewed Jul 20, 2022

View reviewed changes

eddumelendez assigned eddumelendez and kiview Jul 26, 2022

kiview reviewed Jul 28, 2022

View reviewed changes

Comment thread docs/modules/databases/cassandra.md Outdated

eddumelendez and others added 2 commits July 28, 2022 10:16

Update docs/modules/databases/cassandra.md

f8fb105

Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>

example removed and add split tests for drivers

e354cb4

eddumelendez requested a review from kiview July 28, 2022 18:10

eddumelendez mentioned this pull request Aug 1, 2022

Add YugabyteDB module #4372

Merged

16 tasks

bsideup approved these changes Aug 4, 2022

View reviewed changes

kiview approved these changes Aug 4, 2022

View reviewed changes

kiview merged commit c38ba11 into testcontainers:master Aug 4, 2022

kiview removed the type/docs label Aug 19, 2022

akhaku mentioned this pull request Oct 14, 2022

Catch and ignore more errors when reflecting into container subclass #5990

Merged

akhaku mentioned this pull request Oct 14, 2022

Move Cassandra env variable setup to protected method #5991

Closed

	public static final String DEFAULT_LOCAL_DATACENTER = "datacenter1";
	private static final String DEFAULT_LOCAL_DATACENTER = "datacenter1";


		private static final String TEST_CLUSTER_NAME_IN_CONF = "Test Cluster Integration Test";

		private static final String BAISC_QUERY = "SELECT release_version FROM system.local";

Uh oh!

Conversation

emerkle826 commented Jun 2, 2020

Uh oh!

rnorth commented Jul 4, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emerkle826 commented Jul 31, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stale Bot commented Jan 24, 2021

Uh oh!

michaelyaakoby commented Jan 24, 2021

Uh oh!

pluttrell commented Feb 7, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsideup Feb 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kiview commented Aug 4, 2022

Uh oh!

nikeee commented Aug 4, 2022

Uh oh!

akhaku commented Sep 7, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

14 participants

bsideup Feb 8, 2021 •

edited

Loading