absurdfarce commented on code in PR #1958:
URL:
https://github.com/apache/cassandra-java-driver/pull/1958#discussion_r1757128137
##########
test-infra/src/main/java/com/datastax/oss/driver/api/testinfra/ccm/CcmBridge.java:
##########
@@ -61,7 +62,7 @@ public class CcmBridge implements AutoCloseable {
public static final String BRANCH = System.getProperty("ccm.branch");
- public static final Boolean DSE_ENABLEMENT = Boolean.getBoolean("ccm.dse");
+ public static BackendType distribution = null;
Review 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.
##########
test-infra/src/main/java/com/datastax/oss/driver/api/testinfra/ccm/DistributionCassandraVersions.java:
##########
@@ -0,0 +1,55 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package com.datastax.oss.driver.api.testinfra.ccm;
+
+import com.datastax.oss.driver.api.core.Version;
+import com.datastax.oss.driver.api.testinfra.requirement.BackendType;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.TreeMap;
+
+/** Defines mapping of various distributions to shipped Apache Cassandra
version. */
+public abstract class DistributionCassandraVersions {
+ private static final Map<BackendType, TreeMap<Version, Version>> mappings =
new HashMap<>();
+
+ static {
+ {
+ // DSE
+ TreeMap<Version, Version> dse = new TreeMap<>();
+ dse.put(Version.V1_0_0, CcmBridge.V2_1_19);
+ 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);
Review 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,...)
```
##########
integration-tests/src/test/java/com/datastax/dse/driver/api/core/graph/remote/GraphTraversalRemoteITBase.java:
##########
@@ -643,9 +644,9 @@ public void should_allow_use_of_dsl_graph_binary() {
*/
@Test
public void should_return_correct_results_when_bulked() {
- Optional<Version> dseVersion = ccmRule().getCcmBridge().getDseVersion();
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)
Review 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.
##########
integration-tests/src/test/java/com/datastax/dse/driver/api/core/graph/statement/GraphTraversalITBase.java:
##########
@@ -598,7 +598,7 @@ public void should_allow_use_of_dsl_graph_binary() throws
Exception {
@Test
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)
Review 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.
##########
test-infra/src/main/java/com/datastax/oss/driver/api/testinfra/ccm/CcmBridge.java:
##########
@@ -101,22 +102,30 @@ public class CcmBridge implements AutoCloseable {
createTempStore(DEFAULT_SERVER_LOCALHOST_KEYSTORE_PATH);
// major DSE versions
- private static final Version V6_0_0 = Version.parse("6.0.0");
- private static final Version V5_1_0 = Version.parse("5.1.0");
- private static final Version V5_0_0 = Version.parse("5.0.0");
+ public static final Version V6_0_0 = Version.parse("6.0.0");
+ public static final Version V5_1_0 = Version.parse("5.1.0");
+ public static final Version V5_0_0 = Version.parse("5.0.0");
// mapped C* versions from DSE versions
- private static final Version V4_0_0 = Version.parse("4.0.0");
- private static final Version V3_10 = Version.parse("3.10");
- private static final Version V3_0_15 = Version.parse("3.0.15");
- private static final Version V2_1_19 = Version.parse("2.1.19");
+ public static final Version V4_0_0 = Version.parse("4.0.0");
+ public static final Version V3_10 = Version.parse("3.10");
+ public static final Version V3_0_15 = Version.parse("3.0.15");
+ public static final Version V2_1_19 = Version.parse("2.1.19");
+
+ // mapped C* versions from HCD versions
+ public static final Version V4_0_11 = Version.parse("4.0.11");
static {
- if (DSE_ENABLEMENT) {
- LOG.info("CCM Bridge configured with DSE version {}", VERSION);
- } else {
- LOG.info("CCM Bridge configured with Apache Cassandra version {}",
VERSION);
+ distribution = BackendType.CASSANDRA; // default distribution
+ for (BackendType backendType : BackendType.values()) {
+ String enableFlag = "ccm." + backendType.name().toLowerCase();
+ // look for system properties like 'ccm.dse', 'ccm.hcd' etc.
+ if (Boolean.parseBoolean(System.getProperty(enableFlag, "false"))) {
+ distribution = backendType;
+ break;
+ }
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
