-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add new CassandraContainer implementation #8616
Add new CassandraContainer implementation #8616
Conversation
This allows a full support of Cassandra 5 without errors/warnings
1aaefd8
to
437abf6
Compare
Hi @maximevw, thanks for submitting a PR and sorry for missing the discussion 😢 Currently, the |
Hello @eddumelendez, thanks for your response. Yes, it should be possible to run CQL scripts without using the Java driver thanks to So, executing a |
@maximevw Sounds like a great suggestion, do you want to have a stab at this approach? @eddumelendez What is the motivation behind doing this in a new version, rather than in the old version? It is not really a breaking change, is it? |
@kiview Sure, I'll take a look at this asap. |
Current CassandraContainer implementation relies on Cluster class from cassandra java driver. So, the dependency could not be removed at all without causing breaking changes. |
After discussing more with @eddumelendez, it seems indeed a good idea to deprecate |
This implementation in the new package org.testcontainers.cassandra uses cqlsh command inside the running Cassandra container to execute scripts. So, the dependency to the Java driver is no more required. The implementation in the package org.testcontainers.containers has been marked as deprecated.
Hello @kiview @eddumelendez, As previously discussed, I updated this pull request to implement I let you review the changes. |
Thanks for the quick update, @maximevw ! Can you please run |
Sure @eddumelendez! |
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.
Thanks for your contribution! Existing implementation shouldn't be touched to support Cassandra 5. That's why the new implementation is added. The new implementation solved two issues:
- implementation doesn't rely on cassandra driver
- transitive dependency to cassandra driver could be removed
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
- revert changes on org.testcontainers.containers package except deprecation annotations - remove unnecessary genericity on new implementation
@eddumelendez I pushed the changes following your review. |
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.
Small comments. Will perform a deep review later today.
modules/cassandra/src/test/java/org/testcontainers/cassandra/CassandraContainerTest.java
Outdated
Show resolved
Hide resolved
modules/cassandra/src/test/java/org/testcontainers/cassandra/CassandraContainerTest.java
Outdated
Show resolved
Hide resolved
@eddumelendez Changes done :) |
Hello @eddumelendez, Did you have time to finalize your review? 🙂 |
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.
Sorry for the delay, I've left a couple of comments.
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
...cassandra/src/main/java/org/testcontainers/cassandra/delegate/CassandraDatabaseDelegate.java
Show resolved
Hide resolved
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Show resolved
Hide resolved
and do some refactoring
@eddumelendez I pushed the requested changes and replied to your comment about the username/password. 🙂 |
Hello @eddumelendez, I assume you're currently busy, but I'm still interested in finalizing this pull request. Do not hesitate to take a look to the latest updates and comments and let me know if any further changes are required. |
@maximevw thanks for the understading, I'll take a look soon. |
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.
Thanks for your patience, @maximevw! We are getting close, I left one important question in order to make the implementation simple for the users and avoid reasoning about adding wait strategies in certain scenarios, ofc sometimes it has to be done but in this case I think we can make it happen.
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Show resolved
Hide resolved
modules/cassandra/src/main/java/org/testcontainers/cassandra/CassandraContainer.java
Outdated
Show resolved
Hide resolved
…assandraContainer.java Co-authored-by: Eddú Meléndez Gonzales <eddu.melendez@gmail.com>
Thanks for your contribution, @maximevw ! |
This allows a full support of Cassandra 5 without errors/warnings. Indeed, as explained in the discussion #7786, the Cassandra module is still using the deprecated version 3.x of the Java driver for Cassandra. Even if it works with Cassandra 5, it may lead to warnings/errors when using specific features of Cassandra 5. By upgrading the driver to the latest version, we remain compatible with all versions of Cassandra from 3 to 5 (note that version 3 is now reaching EoL).
I successfully tested a locally built version of the Cassandra module including the submitted changes in the tests of Cassandra JDBC wrapper without impact on the performance of the tests running testcontainers with Cassandra.
Few notes regarding the submitted changes:
withJmxReporting(boolean)
.