Backport #791 and remove ambiguity in Cassandra and DSE versions in tests by tolbertam · Pull Request #805 · apache/cassandra-java-driver

tolbertam · 2017-02-14T14:37:38Z

Backports test changes from #791 and also removes ambiguity in Cassandra and DSE versions in tests (see b2e51ab for details).

If accepted, will need to make changes on each upstream branch as we go upwards. I've tested this change on all upstream branches and made the individual fixes, so I should be able to do that quickly when the time comes.

@tolbertam

Motivation: Many tests are failing when using recent versions of C* or DSE. Modifications: 1) Bump the default C* version to 3.8 2) Redesign @CassandraVersion and @DseVersion to be able to fully accept tick-tock releases and correctly compare them. 3) Reduce log verbosity for Scassandra tests 4) Prevent thrift from being enabled for C* >= 4.0 5) Fix decommission tests with C* >= 3.12 This commit contains contributions by Andrew Tolbert (@tolbertam). Result: All tests should pass.

adutra

I love your ideas! I just wonder if we go down this path, why not go even further and distinguish DSE from C* from the very top of the stack, i.e. starting with system properties themselves (see my comment in the review).

adutra · 2017-02-15T15:05:08Z

    private static final Logger logger = LoggerFactory.getLogger(CCMBridge.class);

-    private static final String CASSANDRA_VERSION;
+    private static final String INPUT_CASSANDRA_VERSION;


This is actually only used in the static block below and could be turned into a local variable.

adutra · 2017-02-15T15:10:18Z


    static {
-        CASSANDRA_VERSION = System.getProperty("cassandra.version");
+        INPUT_CASSANDRA_VERSION = System.getProperty("cassandra.version");


What about distinguishing two different system properties for default versions to use: one for OSS C*, one for DSE?

One could e.g. specify that tests requiring DSE should by default use 5.0.3, but tests requiring OSS C* should rather use 3.10.

In this case, the dse system property is useless and the mapping from a DSE version to an OSS C* one should only be used when determining the corresponding C* version of a DSE version (which might be different than the "global" C* version).

I can give it a try myself if you don't mind.

I didn't want to change the existing system properties, I'll think on this for a bit

Discussion for system properties is on the consistentTestVersioning-adu branch didn't want to address that here.

adutra · 2017-02-15T15:11:12Z

-    private static final String CASSANDRA_VERSION;
+    private static final String INPUT_CASSANDRA_VERSION;
+
+    private static final VersionNumber CASSANDRA_VERSION_NUMBER;


How about calling these constants GLOBAL_XXX to match their getter method names?

adutra · 2017-02-15T15:22:56Z

+     *
+     * @return The version of this CCM cluster.
+     */
+    VersionNumber getDSEVersion();


Not sure how much it makes sense to have a CCM cluster instance reporting two different versions. A CCMAccess is a bridge to either a DSE cluster or a C* cluster, not both.

Oh my bad, I see it now, getCassandraVersion() is filled automatically with the corresponding C* version if this instance is a DSE cluster, correct? If so, please disregard my previous comment.

👍 in the case the dse version is provided, the cassandra version is just the derived cassandra verson from dse for deriving test behavior based on that version. I'll update the comments to reflect that.

adutra · 2017-02-15T15:26:44Z

+    /**
+     * @return The desired target protocol version based on the 'cassandra.version' System property.
+     */
+    ProtocolVersion getDesiredProtocolVersion();


Suggestion: replace "desired" – its meaning is quite unclear here – with "target", e.g. getTargetProtocolVersion os even simply getProtocolVersion.
The javadocs need to be updated, here the returned version is the version that matches this specific instance, not the global system property.

I would describe it as the highest version common to the driver and the server, is that accurate?

Yep, that is the intent of this

Ah I see that the documentation doesn't completely reflect that, i'll fix that up:

The target protocol version based on the 'cassandra.version' System property.

adutra · 2017-02-15T15:28:21Z


+    @Override
+    public ProtocolVersion getDesiredProtocolVersion() {
+        VersionNumber version = getCassandraVersion();


this is only checking cassandra versions, shouldn't it be checking dse versions as well? Anyway as per my previous comment I think a single CCMBridge instance should carry only one version, and a boolean flag to indicate if it is a DSE cluster or not.

For now the Cassandra version (which is the compatible cassandra version when DSE version is provided) is adequate for determining the protocol version.

adutra · 2017-02-15T15:31:01Z

         */
-        public Builder withoutDSE() {
-            this.isDSE = false;
+        public Builder withCassandraVersion(VersionNumber versionNumber) {


Again, not sure this makes sense, since withCassandraVersion and withDSEVersion are mutually exclusive (see assertions) why not keep just one method withVersion?

Agreed, reverting this back as it isn't necessary in this particular case.

tolbertam · 2017-02-15T21:20:58Z

woops, didn't mean to delete this 🤦‍♂️

adutra

Looks great, only minor remarks 👍

adutra · 2017-02-16T13:44:20Z

        @Override
-        @SuppressWarnings("SimplifiableIfStatement")
        public boolean equals(Object o) {
-            // do not include cluster name and start, only


I think we should keep the comment about cluster name because indeed a cluster name is unique, but two CCM instances should still be considered equal if their characteristics are identical. As for the start property, I am not sure anymore, but I think we can include it the computation of equals() and hashcode().

Good catch will fix

Hrmm it doesn't look like cluster name is even part of the builder anymore since this is randomly generated, so maybe that comment isn't relevant anymore?

Ah right, silly me :)

adutra · 2017-02-16T13:45:38Z

    /**
     * Helper for generating a DynamicCompositeType {@link ByteBuffer} from the given parameters.
-     *
+     * <p>


This is minor but I think the convention is to use <p/>

Ah, good catch, IntelliJ's autoformatter behaves differently between when you have JDK 8 configured and otherwise. For some reason with JDK 8 it omits /. I'll fix it.

Motivation: Originally CCMBridge was developed targeting running Apache Cassandra clusters using CCM. Over time this has changed to also work with DataStax Enterprise. There are some cases where there was ambiguity with regards to whether the DSE or Cassandra version was used. This attempts to fix this. Also, there were some places where the globally configured Cassandra/DSE version was used (passed in via -Dcassandra.version) instead of a locally configured one for a particular test. This commit also fixes this. In Addition, TestUtils.getDesiredProtocolVersion was made available to determine the ProtocolVersion to use based on the globally configured Cassandra version. This functionality has been moved to CCMAccess, so one can get the ProtocolVersion tied to the version of Cassandra used scoped to the test. Finally, update default cassandra.version used in tests to 3.10. Modifications: Break out CCMAccess.getVersion() into two methods, CCMAccess.getCassandraVersion() and CCMAccess.getDSEVersion(). If DSE is not configured getDSEVersion() returns null, if it is configured getCassandraVersion() returns the mapped version of Cassandra from DSE. Move TestUtils.getDesiredProtocolVersion to CCMAccess. Rename CCMBridge.CASSANDRA_VERSION to CCMBridge.INPUT_CASSANDRA_VERSION. Add CCMBridge.CASSANDRA_VERSION_NUMBER and CCMBridge.DSE_VERSION_NUMBER. Rename CCMBridge.getCassandraVersion() and getDSEVersion() to CCMBridge.getGlobalCassandraVersion() and CCMBridge.getGlobalDSEVersion(). These methods return VersionNumber instead of String. Result: Ambiguity removed between Cassandra and DSE versions. Annotations that call out CCM remain unchanged.

tolbertam · 2017-02-17T17:12:26Z

Squashed the Additional Feedback commits into 202770e, will merge this later today (will let it run through CI for an additional sanity check).

adutra requested changes Feb 15, 2017

View reviewed changes

tolbertam closed this Feb 15, 2017

tolbertam deleted the consistentTestVersioning branch February 15, 2017 21:19

tolbertam reopened this Feb 15, 2017

adutra requested changes Feb 16, 2017

View reviewed changes

adutra approved these changes Feb 16, 2017

View reviewed changes

tolbertam force-pushed the consistentTestVersioning branch from 9a61173 to 202770e Compare February 17, 2017 17:11

tolbertam merged commit a7044d5 into 3.0.x Feb 17, 2017

Conversation

tolbertam commented Feb 14, 2017

Uh oh!

adutra left a comment

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!

Choose a reason for hiding this comment

Uh oh!

tolbertam Feb 15, 2017 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tolbertam commented Feb 15, 2017

Uh oh!

adutra left a comment

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!

tolbertam commented Feb 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tolbertam Feb 15, 2017 •

edited

Loading

tolbertam commented Feb 17, 2017 •

edited

Loading