-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update Cassandra driver to 4.x #2830
Update Cassandra driver to 4.x #2830
Conversation
8b4144d
to
71b86b5
Compare
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. |
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(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 anInetSocketAddress
, and agetDataCenter()
method that returns theDATACENTER
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
71b86b5
to
8803294
Compare
@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 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. |
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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
* @return The configured local Datacenter name. | ||
* @see #withLocalDatacenter(java.lang.String) | ||
*/ | ||
public String getLocalDatacenter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice that you added it, it was missing before.
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. |
I'm using the changes from this pull-request for many months already so we can test using latest cassandra driver. Maybe I or others can help with this? |
@rnorth Soft ping. Wondering what it would take to get this to the finish line? I'd love to start using it. |
private static final String CONTAINER_CONFIG_LOCATION = "/etc/cassandra"; | ||
private static final String USERNAME = "cassandra"; | ||
private static final String PASSWORD = "cassandra"; | ||
|
||
private String configLocation; | ||
private String initScriptPath; | ||
private boolean enableJmxReporting; | ||
private String localDatacenter; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -29,13 +30,15 @@ | |||
public static final String IMAGE = "cassandra"; | |||
|
|||
public static final Integer CQL_PORT = 9042; | |||
public static final String DEFAULT_LOCAL_DATACENTER = "datacenter1"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public static final String DEFAULT_LOCAL_DATACENTER = "datacenter1"; | |
private static final String DEFAULT_LOCAL_DATACENTER = "datacenter1"; |
@@ -166,14 +178,23 @@ public String getPassword() { | |||
} | |||
|
|||
/** | |||
* Get configured Cluster | |||
* Get configured Session |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
* Get configured Session | |
* Get configured Cluster |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this only applies to v4, while in v3 (the currently used types that we're deprecating) it remains Cluster
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops you are right, my bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, one of my first commits for this changed getCluster()
to getSession()
, but that was reverted. I just forgot to revert the comment change.
@@ -25,8 +25,7 @@ | |||
@Override | |||
protected Session createNewConnection() { | |||
try { | |||
return CassandraContainer.getCluster(container) | |||
.newSession(); | |||
return CassandraContainer.getCluster(container).newSession(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
let's revert this non-relevant change to reduce the changeset
@@ -23,11 +27,13 @@ | |||
|
|||
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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo, BAISC_QUERY
-> BASIC_QUERY
} | ||
|
||
@Test | ||
public void testMissingLocalDatacenter() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this seems to be testing the driver, not the container. Let's remove it
cccd50e
to
343f937
Compare
343f937
to
728dcdd
Compare
dependencies { | ||
testImplementation 'org.testcontainers:cassandra' | ||
testImplementation 'junit:junit:4.13.2' | ||
// testRuntimeOnly 'com.datastax.cassandra:cassandra-driver-core:3.10.0' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// testRuntimeOnly 'com.datastax.cassandra:cassandra-driver-core:3.10.0' |
Not needed, or?
Co-authored-by: Kevin Wittek <kiview@users.noreply.github.com>
Merged, thank you @emerkle826 and everyone collaborating on this 🙂 |
Thank you! |
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.
This PR updates the Cassandra driver to the newer 4.x version. It should address #2768.