Refactor integration tests to support multiple C* distributions #1958
Conversation
absurdfarce
left a comment
absurdfarce
left a comment
There was a problem hiding this comment.
A few nits here and there but overall I like where this is headed. I'm not a huge fan of the isDistributionOf()/getDistributionVersion() API for reasons discussed in comments within the review. If it's useful I'm happy to meet and talk about that API.
| public static final String BRANCH = System.getProperty("ccm.branch"); | ||
|
|
||
| public static final Boolean DSE_ENABLEMENT = Boolean.getBoolean("ccm.dse"); | ||
| public static BackendType distribution = null; |
There was a problem hiding this comment.
Semi-nit: this should be DISTRIBUTION to be consistent with the other static variables in this class. I don't normally get too excited about style things like this but in this case we use all cap for static var names so that they can easily be identified when trawling through code.
| if (Boolean.parseBoolean(System.getProperty(enableFlag, "false"))) { | ||
| distribution = backendType; | ||
| break; | ||
| } |
There was a problem hiding this comment.
At this point I'm starting to wonder if it isn't just easier to support a single env var whereby you can specify the backend you want.
ccm.backend=dse
ccm.backend=hcd
etc.
There was a problem hiding this comment.
Totally agreed. I wanted to preserve backward compatibility of flags, but true this makes far more sense.
| dse.put(Version.V5_0_0, CcmBridge.V3_0_15); | ||
| dse.put(CcmBridge.V5_1_0, CcmBridge.V3_10); | ||
| dse.put(CcmBridge.V6_0_0, CcmBridge.V4_0_0); | ||
| mappings.put(BackendType.DSE, dse); |
There was a problem hiding this comment.
Nit: Guava's ImmutableMap builder make this a bit cleaner:
Map<Version, Version> dse = ImmutableMap.of(Version.V1_0_0, CcmBridge.V2_1_19, Version.V5_0_0, CcmBridge.V3_0_15,...)
There was a problem hiding this comment.
It needed to be TreeMap, so changed to ImmutableSortedMap.
| public void should_return_correct_results_when_bulked() { | ||
| Assumptions.assumeThat( | ||
| ccmRule().getCcmBridge().getDseVersion().get().compareTo(Version.parse("5.1.2")) > 0) | ||
| ccmRule().getCcmBridge().getDistributionVersion().compareTo(Version.parse("5.1.2")) > 0) |
There was a problem hiding this comment.
This seems like a change in logic. Before we were getting the DSE version specifically and comparing it to the min version we want to support for this test. Here we're getting a "distribution version" (which could be anything) and checking to see if it exceeds the min version. We should somehow be validating that we're dealing with a DSE version when we get it.
Note that GraphTraversalRemoteITBase above does exactly the same check... but it makes sure we're dealing with a DSE distribution before we continue on.
There was a problem hiding this comment.
Thank you for catching this. I do not know how I overlooked it. Fixed.
| Assumptions.assumeThat( | ||
| dseVersion.isPresent() && dseVersion.get().compareTo(Version.parse("5.1.2")) > 0) | ||
| CcmBridge.isDistributionOf(BackendType.DSE) | ||
| && CcmBridge.getDistributionVersion().compareTo(Version.parse("5.1.2")) > 0) |
There was a problem hiding this comment.
This feels like we're just moving the problem around a bit. We haven't decreased the verbosity... we've just moved it around a bit.
There are a lot of instances of things that use this pattern:
Are we DSE/OSS/HCD && are we greater than/less than/something else some specific version
The removal of the Optional types here (i.e. the removal of CcmBridge.getDseVersion() below) means that we have to remember to do both of the checks above manually. If we were to make better use of the Optional type, however, we could do the whole thing in a single check. For example, the statements above can also be expressed as:
Assumptions.assumeThat(
ccmRule().getCcmBridge().getDseVersion().map(v -> v.compareTo(Version.parse("5.1.2")) > 0).orElse(false))
.isTrue();
In this case if getDseVersion() returns an empty Optional (because we aren't dealing with DSE) the orElse() will return false. Otherwise we do the integer compare and return true or false based on it's results. This approach allows us to encapsulate all the checks we want into a single line of logic. I'd argue that's more desirable in a test framework like this where you want to make sure your base assumptions are respected before moving on to the actual tests.
| public static boolean isDistributionOf(BackendType type, VersionComparator comparator) { | ||
| return isDistributionOf(type) | ||
| && comparator.accept(getDistributionVersion(), getCassandraVersion()); | ||
| } |
There was a problem hiding this comment.
I like this more since it at least preserves the one-shot comparison of backend type + version but overall I would still argue for something built around the Optional type. But this PR has been held up long enough as-is and I don't feel strongly enough about it to hold things up any longer so we'll roll with this.
|
Thanks for sticking with this @lukasz-antoniak! Can you resolve the conflicts above? I'd like to get a Jenkins run of this branch to make sure we don't have any unexpected regressions but I can't do that with a PR branch since it isn't mergable at the moment. |
… with DataStax HCD 1.0.0
lukasz-antoniak
force-pushed
the
HCD-10-simple
branch
from
b08c01a to
f6beca0
Compare
|
Rebased and squashed commits. |
|
Thanks @lukasz-antoniak! Since we're only dealing with testing fixes (and DataStax-specific testing fixes at that) I'm going to merge this with a single 👍 |
… with DataStax HCD 1.0.0 patch by Lukasz Antoniak; reviewed by Bret McGuire reference: #1958
* CASSANDRA-19635: Run integration tests with C* 5.x patch by Lukasz Antoniak; reviewed by Andy Tolbert, and Bret McGuire for CASSANDRA-19635 * CASSANDRA-19635: Configure Jenkins to run integration tests with C* 5.x patch by Lukasz Antoniak; reviewed by Bret McGuire for CASSANDRA-19635 * update badge URL to org.apache.cassandra/java-driver-core * Limit calls to Conversions.resolveExecutionProfile Those repeated calls account for a non-negligible portion of my application CPU (0.6%) and can definitly be a final field so that it gets resolved only once per CqlRequestHandler. patch by Benoit Tellier; reviewed by Andy Tolbert, and Bret McGuire reference: apache#1623 * autolink JIRA tickets in commit messages patch by Stefan Miklosovic; reviewed by Michael Semb Wever for CASSANDRA-19854 * Don't return empty routing key when partition key is unbound DefaultBoundStatement#getRoutingKey has logic to infer the routing key when no one has explicitly called setRoutingKey or otherwise set the routing key on the statement. It however doesn't check for cases where nothing has been bound yet on the statement. This causes more problems if the user decides to get a BoundStatementBuilder from the PreparedStatement, set some fields on it, and then copy it by constructing new BoundStatementBuilder objects with the BoundStatement as a parameter, since the empty ByteBuffer gets copied to all bound statements, resulting in all requests being targeted to the same Cassandra node in a token-aware load balancing policy. patch by Ammar Khaku; reviewed by Andy Tolbert, and Bret McGuire reference: apache#1620 * JAVA-3167: CompletableFutures.allSuccessful() may return never completed future patch by Lukasz Antoniak; reviewed by Andy Tolbert, and Bret McGuire for JAVA-3167 * ninja-fix Various test fixes * Run integration tests with DSE 6.9.0 patch by Lukasz Antoniak; reviewed by Bret McGuire reference: apache#1955 * JAVA-3117: Call CcmCustomRule#after if CcmCustomRule#before fails to allow subsequent tests to run patch by Henry Hughes; reviewed by Alexandre Dutra and Andy Tolbert for JAVA-3117 * JAVA-3149: Support request cancellation in request throttler patch by Lukasz Antoniak; reviewed by Andy Tolbert and Chris Lohfink for JAVA-3149 * Fix C* 3.0 tests failing on Jenkins patch by Lukasz Antoniak; reviewed by Bret McGuire reference: apache#1939 * Reduce lock held duration in ConcurrencyLimitingRequestThrottler It might take some (small) time for callback handling when the throttler request proceeds to submission. Before this change, the throttler proceed request will happen while holding the lock, preventing other tasks from proceeding when there is spare capacity and even preventing tasks from enqueuing until the callback completes. By tracking the expected outcome, we can perform the callback outside of the lock. This means that request registration and submission can proceed even when a long callback is being processed. patch by Jason Koch; Reviewed by Andy Tolbert and Chris Lohfink for CASSANDRA-19922 * Annotate BatchStatement, Statement, SimpleStatement methods with CheckReturnValue Since the driver's default implementation is for BatchStatement and SimpleStatement methods to be immutable, we should annotate those methods with @CheckReturnValue. Statement#setNowInSeconds implementations are immutable so annotate that too. patch by Ammar Khaku; reviewed by Andy Tolbert and Bret McGuire reference: apache#1607 * Remove "beta" support for Java17 from docs patch by Bret McGuire; reviewed by Andy Tolbert and Alexandre Dutra reference: apache#1962 * Fix uncaught exception during graceful channel shutdown after exceeding max orphan ids patch by Christian Aistleitner; reviewed by Andy Tolbert, and Bret McGuire for apache#1938 * Build a public CI for Apache Cassandra Java Driver patch by Siyao (Jane) He; reviewed by Mick Semb Wever for CASSANDRA-19832 * CASSANDRA-19932: Allow to define extensions while creating table patch by Lukasz Antoniak; reviewed by Bret McGuire and Chris Lohfink * Fix DefaultSslEngineFactory missing null check on close patch by Abe Ratnofsky; reviewed by Andy Tolbert and Chris Lohfink for CASSANDRA-20001 * Query builder support for NOT CQL syntax patch by Bret McGuire; reviewed by Bret McGuire and Andy Tolbert for CASSANDRA-19930 * Fix CustomCcmRule to drop `CURRENT` flag no matter what If super.after() throws an Exception `CURRENT` flag is never dropped which leads next tests to fail with IllegalStateException("Attempting to use a Ccm rule while another is in use. This is disallowed") Patch by Dmitry Kropachev; reviewed by Andy Tolbert and Bret McGuire for JAVA-3117 * JAVA-3051: Memory leak patch by Jane He; reviewed by Alexandre Dutra and Bret McGuire for JAVA-3051 * Automate latest Cassandra versions when running CI patch by Siyao (Jane) He; reviewed by Mick Semb Wever for CASSJAVA-25 * Refactor integration tests to support multiple C* distributions. Test with DataStax HCD 1.0.0 patch by Lukasz Antoniak; reviewed by Bret McGuire reference: apache#1958 * Fix TableMetadata.describe() when containing a vector column patch by Stefan Miklosovic; reviewed by Bret McGuire for CASSJAVA-2 * Move Apache Cassandra 5.x off of beta1 and remove some older Apache Cassandra versions. patch by Bret McGuire; reviewed by Bret McGuire for CASSJAVA-54 * Update link to Jira to be CASSJAVA Updating the link to Jira. Previously we had a component in the CASSANDRA Jira project but now we have a project for each driver - in the case of Java, it's CASSJAVA. Added CASSJAVA to .asf.yaml patch by Jeremy Hanna; reviewed by Bret McGuire for CASSJAVA-61 * Move DataStax shaded Guava module into Java driver patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-52 * JAVA-3057 Allow decoding a UDT that has more fields than expected patch by Ammar Khaku; reviewed by Andy Tolbert and Bret McGuire reference: apache#1635 * CASSJAVA-55 Remove setting "Host" header for metadata requests. With some sysprops enabled this will actually be respected which completely borks Astra routing. patch by Bret McGuire; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-55 * JAVA-3118: Add support for vector data type in Schema Builder, QueryBuilder patch by Jane He; reviewed by Mick Semb Wever and Bret McGuire for JAVA-3118 reference: apache#1931 * Upgrade Guava to 33.3.1-jre patch by Lukasz Antoniak; reviewed by Alexandre Dutra and Bret McGuire for CASSJAVA-53 * Do not always cleanup Guava shaded module before packaging * Revert "Do not always cleanup Guava shaded module before packaging" This reverts commit 5be52ec. * Conditionally compile shaded Guava module * JAVA-3143: Extend driver vector support to arbitrary subtypes and fix handling of variable length types (OSS C* 5.0) patch by Jane He; reviewed by Bret McGuire and João Reis reference: apache#1952 * JAVA-3168 Copy node info for contact points on initial node refresh only from first match by endpoint patch by Alex Sasnouskikh; reviewed by Andy Tolbert and Alexandre Dura for JAVA-3168 * JAVA-3055: Prevent PreparedStatement cache to be polluted if a request is cancelled. There was a critical issue when the external code cancels a request, indeed the cached CompletableFuture will then always throw a CancellationException. This may happens, for example, when used by reactive like Mono.zip or Mono.firstWithValue. patch by Luc Boutier; reviewed by Alexandre Dutra and Bret McGuire reference: apache#1757 * Expose a decorator for CqlPrepareAsyncProcessor cache rather than the ability to specify an arbitrary cache from scratch. Also bringing tests from apache#2003 forward with a few minor changes due to this implementation patch by Bret McGuire; reviewed by Bret McGuire and Andy Tolbert reference: apache#2008 * ninja-fix Using shaded Guava classes for import in order to make OSGi class paths happy. Major hat tip to Dmitry Konstantinov for the find here! * Changelog updates for 4.19.0 * [maven-release-plugin] prepare release 4.19.0 * [maven-release-plugin] prepare for next development iteration * Specify maven-clean-plugin version Sets the version to 3.4.1 in parent pom. Having it unspecified causes the following warning: ``` [WARNING] Some problems were encountered while building the effective model for com.scylladb:java-driver-guava-shaded:jar:4.19.0.0-SNAPSHOT [WARNING] 'build.plugins.plugin.version' for org.apache.maven.plugins:maven-clean-plugin is missing. @ line 97, column 15 [WARNING] [WARNING] It is highly recommended to fix these problems because they threaten the stability of your build. [WARNING] [WARNING] For this reason, future Maven versions might no longer support building such malformed projects. [WARNING] ``` * Install guava-shaded before running core's compile in CI Without this module available "Build" and "Unit tests" job fail with "package <x> does not exist" or "cannot find symbol" pointing to `[...].shaded.guava.[...]` packages. * Remove exception catch in `prepared_stmt_metadata_update_loopholes_test` Since incorporating "JAVA-3057 Allow decoding a UDT that has more fields than expected" the asuumptions of removed check are no longer valid. * Switch shaded guava's groupId in osgi-tests Switches from `org.apache.cassandra` to `com.scylladb` in `BundleOptions#commonBundles()`. * Typo: increase line's loglevel in CcmBridge --------- Co-authored-by: Lukasz Antoniak <[email protected]> Co-authored-by: Brad Schoening <[email protected]> Co-authored-by: Benoit Tellier <[email protected]> Co-authored-by: Stefan Miklosovic <[email protected]> Co-authored-by: Ammar Khaku <[email protected]> Co-authored-by: absurdfarce <[email protected]> Co-authored-by: Henry Hughes <[email protected]> Co-authored-by: Jason Koch <[email protected]> Co-authored-by: Christian Aistleitner <[email protected]> Co-authored-by: janehe <[email protected]> Co-authored-by: Abe Ratnofsky <[email protected]> Co-authored-by: absurdfarce <[email protected]> Co-authored-by: Dmitry Kropachev <[email protected]> Co-authored-by: janehe <[email protected]> Co-authored-by: Jeremy Hanna <[email protected]> Co-authored-by: SiyaoIsHiding <[email protected]> Co-authored-by: Alex Sasnouskikh <[email protected]> Co-authored-by: Luc Boutier <[email protected]>
2 participants
Refactored integration tests to be able to support multiple C* distributions (not only DSE vs. OSS).