frankgh commented on code in PR #48:
URL: https://github.com/apache/cassandra-sidecar/pull/48#discussion_r1224552509
##########
src/test/java/org/apache/cassandra/sidecar/routes/RingHandlerTest.java:
##########
@@ -187,9 +187,9 @@ static class RingHandlerTestModule extends AbstractModule
@Singleton
public InstancesConfig instanceConfig() throws IOException
{
- final int instanceId = 100;
- final String host = "127.0.0.1";
- final InstanceMetadata instanceMetadata =
mock(InstanceMetadata.class);
+ int instanceId = 100;
+ String host = "localhost";
Review Comment:
why did this change? I'm wondering why unit tests needed to change. I would
expect only integration tests to fail.
##########
adapters/latest/src/main/java/org/apache/cassandra/sidecar/adapters/latest/CassandraStorageOperations.java:
##########
@@ -64,8 +73,28 @@ public void takeSnapshot(@NotNull String tag, @NotNull
String keyspace, @NotNull
requireNonNull(tag, "snapshot tag must be non-null");
requireNonNull(keyspace, "keyspace for the must be non-null");
requireNonNull(table, "table must be non-null");
- jmxClient.proxy(StorageJmxOperations.class, STORAGE_SERVICE_OBJ_NAME)
- .takeSnapshot(tag, options, keyspace + "." + table);
+ try
+ {
+ jmxClient.proxy(StorageJmxOperations.class,
STORAGE_SERVICE_OBJ_NAME)
+ .takeSnapshot(tag, options, keyspace + "." + table);
+ }
+ catch (IOException e)
+ {
+ if (StringUtils.contains(e.getMessage(),
+ "Snapshot " + tag + " already exists"))
+ {
+ throw new SnapshotAlreadyExistsException(e.getMessage());
+ }
+ else if (StringUtils.contains(e.getMessage(), "Keyspace " +
keyspace + " does not exist"))
+ {
+ throw new IllegalArgumentException(e.getMessage());
+ }
+ else if (StringUtils.contains(e.getMessage(),
+ "Cannot snapshot until bootstrap
completes"))
+ {
+ throw new NodeBootstrappingException(e.getMessage());
+ }
Review Comment:
you are swallowing any other exception error message. We should propagate it
anyway
```suggestion
}
throw new Runtime(e);
```
##########
src/test/java/org/apache/cassandra/sidecar/routes/SchemaHandlerTest.java:
##########
@@ -165,7 +165,7 @@ public class SchemaHandlerTestModule extends AbstractModule
public InstancesConfig instanceConfig() throws IOException
{
final int instanceId = 100;
- final String host = "127.0.0.1";
+ final String host = "localhost";
Review Comment:
revert this change?
##########
src/test/java/org/apache/cassandra/sidecar/routes/ExtractHostAddressWithoutPortTest.java:
##########
@@ -30,15 +30,15 @@
@Test
void testAddressWithIPv4Host()
{
- final String host =
AbstractHandler.extractHostAddressWithoutPort("127.0.0.1");
- assertEquals("127.0.0.1", host);
+ final String host =
AbstractHandler.extractHostAddressWithoutPort("localhost");
Review Comment:
revert changes in this file? this is actually changing the test as described
by the name of the test
##########
README.md:
##########
@@ -27,9 +45,71 @@ While setting up cassandra instance, make sure the data
directories of cassandra
Testing
-------
-We rely on docker containers for integration tests.
+The test framework is set up to run 4.1 and 5.0 (Trunk) tests (see
`TestVersionSupplier.java`) by default.
+You can change this via the Java property `cassandra.sidecar.versions_to_test`
by supplying a comma-delimited string.
+For example, `-Dcassandra.sidecar.versions_to_test=4.0,4.1,5.0`.
-The only requirement is to install and run
[Docker](https://www.docker.com/products/docker-desktop/) on your test machine.
+In order for tests to run successfully under JDK11, you'll need to add the
following JVM arguments to your test runner of choice.
+You should also set your test framework to fork a new process at least every
class, if not every method, as there are still
+a few unresolved memory-related issues in the in-jvm dtest framework.
+```
+-Djdk.attach.allowAttachSelf=true
+-XX:+UseConcMarkSweepGC
Review Comment:
yeah, I think this is unnecessary. Don't we already have these in
build.gradle? Someone interested in building it manually without using build
tools can look inside build.gradle?
##########
adapters/latest/build.gradle:
##########
@@ -18,10 +19,11 @@ test {
}
dependencies {
- implementation(project(":common"))
+ api(project(":common"))
+ api("com.google.guava:guava:${project.rootProject.guavaVersion}")
compileOnly('org.jetbrains:annotations:23.0.0')
compileOnly('com.datastax.cassandra:cassandra-driver-core:3.11.3')
-
implementation("com.google.guava:guava:${project.rootProject.guavaVersion}")
+ implementation("org.apache.commons:commons-lang3:3.9")
Review Comment:
can we avoid adding this dependency? I generally get very nervous when we
add new dependencies. So I would only add it if it's absolutely necessary.
##########
common/src/main/java/org/apache/cassandra/sidecar/common/ICassandraAdapter.java:
##########
@@ -49,4 +49,5 @@
* @return the {@link TableOperations} implementation for the Cassandra
cluster
*/
TableOperations tableOperations();
+
Review Comment:
remove this line maybe?
##########
src/test/java/org/apache/cassandra/sidecar/routes/sstableuploads/BaseUploadsHandlerTest.java:
##########
@@ -181,7 +181,7 @@ public CassandraAdapterDelegate delegate()
public Configuration abstractConfig(InstancesConfig instancesConfig)
{
when(mockConfiguration.getInstancesConfig()).thenReturn(instancesConfig);
- when(mockConfiguration.getHost()).thenReturn("127.0.0.1");
+ when(mockConfiguration.getHost()).thenReturn("localhost");
Review Comment:
can we revert the change in this file?
##########
src/test/java/org/apache/cassandra/sidecar/routes/GossipInfoHandlerTest.java:
##########
@@ -129,7 +129,7 @@ static class GossipInfoHandlerTestModule extends
AbstractModule
public InstancesConfig instanceConfig() throws IOException
{
final int instanceId = 100;
- final String host = "127.0.0.1";
+ final String host = "localhost";
Review Comment:
revert this change?
##########
adapters/latest/src/main/java/org/apache/cassandra/sidecar/adapters/latest/CassandraAdapter.java:
##########
@@ -0,0 +1,138 @@
+/*
+ * 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 org.apache.cassandra.sidecar.adapters.trunk;
+
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.datastax.driver.core.Metadata;
+import com.datastax.driver.core.Row;
+import com.datastax.driver.core.Session;
+import org.apache.cassandra.sidecar.common.CQLSessionProvider;
+import org.apache.cassandra.sidecar.common.ClusterMembershipOperations;
+import org.apache.cassandra.sidecar.common.ICassandraAdapter;
+import org.apache.cassandra.sidecar.common.JmxClient;
+import org.apache.cassandra.sidecar.common.NodeSettings;
+import org.apache.cassandra.sidecar.common.StorageOperations;
+import org.apache.cassandra.sidecar.common.TableOperations;
+import org.apache.cassandra.sidecar.common.dns.DnsResolver;
+import org.jetbrains.annotations.Nullable;
+
+/**
+ * A {@link ICassandraAdapter} implementation for Cassandra 4.0 and later
+ */
+public class CassandraAdapter implements ICassandraAdapter
Review Comment:
recollecting how the versions work, I think we annotate a factory with
`@MinimumVersion("....")`. So if the minimum version is `4.0.0` that means the
adapter is good for any version 4.0.0+, although there might be breaking
changes in new versions in which case a new `ICassandraFactory` must be
provided.
The provided factory in the code base is good for `4.0.0`, `4.1.0`, and
`5.0.0` looking at the test results. But the implementation we have was based
on the minimum version of 4.0.0, so I think it would make more sense to have it
be the `Cassandra40Factory`, which gives you a `Cassandra40Adapter`.
I think we should test against trunk for sure, this will allow us to detect
any breaking changes and would let us determine whether Sidecar is
_forward-compatible_ with the tip of Cassandra's trunk.
The question is what would be the expectation when latest is no longer
compatible and we need to break it.
- latest remains compatible with the tip of Cassandra's trunk
- We introduce a Cassandra40Adapter then, that will work correctly with
versions 4.0.0 through tip of trunk (not inclusive)
##########
src/test/java/org/apache/cassandra/sidecar/utils/SSTableImporterTest.java:
##########
@@ -67,7 +67,7 @@ public void setup() throws InterruptedException
mockTableOperations1 = mock(TableOperations.class);
TableOperations mockTableOperations2 = mock(TableOperations.class);
when(mockConfiguration.getSSTableImportPollIntervalMillis()).thenReturn(10);
-
when(mockMetadataFetcher.delegate("127.0.0.1")).thenReturn(mockCassandraAdapterDelegate1);
Review Comment:
can we undo the changes to this file? They seem unnecessary and unrelated to
the integration tests. Any reason for these changes?
##########
src/test/java/org/apache/cassandra/sidecar/TestSslModule.java:
##########
@@ -58,7 +58,7 @@ public Configuration abstractConfig(InstancesConfig
instancesConfig)
return new Configuration.Builder()
.setInstancesConfig(instancesConfig)
- .setHost("127.0.0.1")
+ .setHost("localhost")
Review Comment:
revert this change?
--
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]
