You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by GitBox <gi...@apache.org> on 2022/04/06 16:58:40 UTC
[GitHub] [cassandra] josh-mckenzie commented on a diff in pull request #1432: CASSANDRA-17332 Add support for vnodes in jvm-dtest
josh-mckenzie commented on code in PR #1432:
URL: https://github.com/apache/cassandra/pull/1432#discussion_r844133875
##########
test/distributed/org/apache/cassandra/distributed/test/GossipTest.java:
##########
@@ -46,13 +47,13 @@
import org.apache.cassandra.streaming.StreamPlan;
import org.apache.cassandra.streaming.StreamResultFuture;
import org.apache.cassandra.utils.FBUtilities;
+import org.assertj.core.api.Assertions;
Review Comment:
nit: import ordering
##########
test/distributed/org/apache/cassandra/distributed/impl/InstanceConfig.java:
##########
@@ -67,14 +69,15 @@ private InstanceConfig(int num,
String commitlog_directory,
String hints_directory,
String cdc_raw_directory,
- String initial_token,
+ Collection<String> initial_token,
int storage_port,
int native_transport_port)
{
this.num = num;
this.networkTopology = networkTopology;
this.hostId = new UUID(0x4000L, (1L << 63) | num); // deterministic hostId for simulator
- this .set("num_tokens", 1)
+ this .set("num_tokens", initial_token.size())
Review Comment:
Have we considered using public static constants for this, or are they ingrained enough in our collective culture that we're fine just using these param names raw in code + config?
We also reference num_tokens raw in ClusterUtils on getTokenCount (and a variety of other places). Given these are pretty calcified by virtue of being in the .yaml and thus part of our public API contract, probably fine just leaving things. Was just an observation while reading through this (lots of raw strings for params floating around here)
##########
test/distributed/org/apache/cassandra/distributed/impl/InstanceConfig.java:
##########
@@ -208,10 +216,9 @@ public void propagate(Object writeToConfig, Map<Class<?>, Function<Object, Objec
throw new IllegalStateException("In-JVM dtests no longer support propagate");
}
+ @Override
public void validate()
{
Review Comment:
Consider leaving a breadcrumb comment as to why we're leaving this here as a no-op after removing its body (precedent for this in our various ALTER/SELECT/etcStatement.java files in validate())
##########
test/distributed/org/apache/cassandra/distributed/impl/AbstractClusterTest.java:
##########
@@ -0,0 +1,251 @@
+/*
+ * 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.distributed.impl;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Consumer;
+
+import org.junit.AssumptionViolatedException;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.api.IInstanceConfig;
+import org.apache.cassandra.distributed.api.IInvokableInstance;
+import org.apache.cassandra.distributed.api.TokenSupplier;
+import org.apache.cassandra.distributed.impl.AbstractCluster.AbstractBuilder;
+import org.apache.cassandra.distributed.shared.Versions;
+import org.apache.cassandra.utils.FailingRunnable;
+import org.assertj.core.api.Assertions;
+import org.mockito.Mockito;
+
+
+public class AbstractClusterTest
+{
+ @Test
+ public void allowVnodeWithMultipleTokens()
+ {
+ AbstractBuilder builder = builder();
+ builder.withTokenCount(42);
+ unroll(() -> builder.createWithoutStarting());
+ }
+
+ @Test
+ public void allowVnodeWithSingleToken()
+ {
+ AbstractBuilder builder = builder();
+ builder.withTokenCount(1);
+ unroll(() -> builder.createWithoutStarting());
+ }
+
+ @Test
+ public void disallowVnodeWithMultipleTokens()
+ {
+ AbstractBuilder builder = builder();
+
+ builder.withoutVNodes();
+ builder.withTokenCount(42);
+
+ Assertions.assertThatThrownBy(() -> builder.createWithoutStarting())
+ .isInstanceOf(AssumptionViolatedException.class)
+ .hasMessage("vnode is not supported");
+ }
+
+
+ @Test
+ public void disallowVnodeWithSingleToken()
+ {
+ AbstractBuilder builder = builder();
+
+ builder.withoutVNodes();
+ builder.withTokenCount(1);
+
+ unroll(() -> builder.createWithoutStarting());
+ }
+
+ @Test
+ public void withoutVNodes()
+ {
+ AbstractBuilder builder = builder();
+
+ builder.withoutVNodes();
+ //TODO casting is annoying... what can be done to be smarter?
Review Comment:
What do we want to do about these TODO's? Create a follow up JIRA to tend to them, take care of them here, leave them for a future dev to work on?
##########
test/distributed/org/apache/cassandra/distributed/test/PreviewRepairTest.java:
##########
@@ -35,6 +35,7 @@
import com.google.common.util.concurrent.Uninterruptibles;
+import org.apache.cassandra.distributed.shared.ClusterUtils;
Review Comment:
nit: import ordering. Have some o.a.c. above and below org.junit here.
##########
test/distributed/org/apache/cassandra/distributed/test/PaxosRepairTest.java:
##########
@@ -215,7 +215,7 @@ private static void repair(Cluster cluster, String keyspace, String table)
@Test
public void paxosRepairTest() throws Throwable
{
- try (Cluster cluster = init(Cluster.create(3, CONFIG_CONSUMER)))
+ try (Cluster cluster = init(Cluster.build(3).withConfig(CONFIG_CONSUMER).withoutVNodes().start()))
Review Comment:
For the tests where we're explicitly configuring our clusters without vnode support (and will thus be skipped on a vnode enabled run), could we document _why_ we run them without vnodes?
##########
build.xml:
##########
@@ -78,6 +78,7 @@
<property name="test.simulator-test.src" value="${test.dir}/simulator/test"/>
<property name="test.driver.connection_timeout_ms" value="5000"/>
<property name="test.driver.read_timeout_ms" value="12000"/>
+ <property name="test.jvm.args" value="" />
Review Comment:
Could we use a different name here to reflect that? This gives us "test.jvm.args" and "test-jvmargs" which is pretty confusing. Maybe something like "ant.jvm.args" or something?
##########
test/distributed/org/apache/cassandra/distributed/impl/AbstractClusterTest.java:
##########
@@ -0,0 +1,251 @@
+/*
+ * 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.distributed.impl;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.HashMap;
+import java.util.Map;
+import java.util.function.Consumer;
+
+import org.junit.AssumptionViolatedException;
+import org.junit.Test;
+
+import org.apache.cassandra.distributed.api.IInstanceConfig;
+import org.apache.cassandra.distributed.api.IInvokableInstance;
+import org.apache.cassandra.distributed.api.TokenSupplier;
+import org.apache.cassandra.distributed.impl.AbstractCluster.AbstractBuilder;
+import org.apache.cassandra.distributed.shared.Versions;
+import org.apache.cassandra.utils.FailingRunnable;
+import org.assertj.core.api.Assertions;
+import org.mockito.Mockito;
+
+
+public class AbstractClusterTest
+{
+ @Test
+ public void allowVnodeWithMultipleTokens()
+ {
+ AbstractBuilder builder = builder();
+ builder.withTokenCount(42);
+ unroll(() -> builder.createWithoutStarting());
+ }
+
+ @Test
+ public void allowVnodeWithSingleToken()
+ {
+ AbstractBuilder builder = builder();
+ builder.withTokenCount(1);
+ unroll(() -> builder.createWithoutStarting());
+ }
+
+ @Test
+ public void disallowVnodeWithMultipleTokens()
+ {
+ AbstractBuilder builder = builder();
+
+ builder.withoutVNodes();
+ builder.withTokenCount(42);
+
+ Assertions.assertThatThrownBy(() -> builder.createWithoutStarting())
+ .isInstanceOf(AssumptionViolatedException.class)
+ .hasMessage("vnode is not supported");
+ }
+
+
+ @Test
+ public void disallowVnodeWithSingleToken()
+ {
+ AbstractBuilder builder = builder();
+
+ builder.withoutVNodes();
+ builder.withTokenCount(1);
+
+ unroll(() -> builder.createWithoutStarting());
+ }
+
+ @Test
+ public void withoutVNodes()
+ {
+ AbstractBuilder builder = builder();
+
+ builder.withoutVNodes();
+ //TODO casting is annoying... what can be done to be smarter?
+ builder.withTokenSupplier((TokenSupplier) i -> Arrays.asList("a", "b", "c"));
+
+ Assertions.assertThatThrownBy(() -> builder.createWithoutStarting())
+ .isInstanceOf(AssumptionViolatedException.class)
+ .hasMessage("vnode is not supported");
+ }
+
+ @Test
+ public void vnodeButTokensDoNotMatch()
+ {
+ AbstractBuilder builder = builder();
+
+ builder.withTokenCount(1);
+ //TODO casting is annoying... what can be done to be smarter?
+ builder.withTokenSupplier((TokenSupplier) i -> Arrays.asList("a", "b", "c"));
+
+ Assertions.assertThatThrownBy(() -> builder.createWithoutStarting())
+ .isInstanceOf(AssumptionViolatedException.class)
+ .hasMessage("no-vnode is requested but not supported");
+ }
+
+ @Test
+ public void novnodeButTokensDoNotMatch()
Review Comment:
nit: capitalization inconsistency. `noVnodeButTokensDoNotMatch`
##########
test/distributed/org/apache/cassandra/distributed/test/GossipTest.java:
##########
@@ -277,6 +279,13 @@ public void gossipShutdownUpdatesTokenMetadata() throws Exception
}
}
+ private static String getLocalToken(IInvokableInstance node)
+ {
+ Collection<String> tokens = ClusterUtils.getLocalTokens(node);
+ Assertions.assertThat(tokens).hasSize(1);
+ return tokens.stream().findFirst().get();
+ }
+
Review Comment:
Yeah, was wondering if this was more general purpose useful (i.e. should live in some kind of Utils class) rather than restricting it to Gossip only.
Might make sense to move this over adjacent to `ClusterUtils.getLocalTokens` as a general purpose helper function even though we only use it in this one place atm. Otherwise we're more likely to duplicate this logic over there in the future when someone needs it if they don't dig enough to find this.
##########
test/distributed/org/apache/cassandra/distributed/test/ReplicaFilteringProtectionTest.java:
##########
@@ -50,30 +52,43 @@
private static final int ROWS = 3;
private static Cluster cluster;
+ private static String skipMessage;
@BeforeClass
public static void setup() throws IOException
{
- cluster = init(Cluster.build()
- .withNodes(REPLICAS)
- .withConfig(config -> config.set("hinted_handoff_enabled", false)
- .set("commitlog_sync", "batch")
- .set("num_tokens", 1)).start());
-
- // Make sure we start w/ the correct defaults:
- cluster.get(1).runOnInstance(() -> assertEquals(DEFAULT_WARN_THRESHOLD, StorageService.instance.getCachedReplicaRowsWarnThreshold()));
- cluster.get(1).runOnInstance(() -> assertEquals(DEFAULT_FAIL_THRESHOLD, StorageService.instance.getCachedReplicaRowsFailThreshold()));
+ try
+ {
+ cluster = init(Cluster.build()
+ .withNodes(REPLICAS)
+ .withConfig(config -> config.set("hinted_handoff_enabled", false)
+ .set("commitlog_sync", "batch")
+ .set("num_tokens", 1)).start());
Review Comment:
Why does this test need num_tokens set to 1 like this? Seems like this is the cause of needing all the song and dance around capturing the skipMessage etc. Also seems somewhat unique to this test, but I can't think of any reason replica filtering as a mechanism wouldn't need to also be tested in a vnode enabled environment? (caveat: haven't read through all the tests here in detail; spent time reviewing the logic around capturing the skip reasoning 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: pr-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org