You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cassandra.apache.org by "frankgh (via GitHub)" <gi...@apache.org> on 2024/05/11 13:54:31 UTC
[PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
frankgh opened a new pull request, #59:
URL: https://github.com/apache/cassandra-analytics/pull/59
Patch by Francisco Guerrero; Reviewed by Yifan Cai for CASSANDRA-19634
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #59:
URL: https://github.com/apache/cassandra-analytics/pull/59#discussion_r1597630564
##########
cassandra-analytics-integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/SharedClusterIntegrationTestBase.java:
##########
@@ -486,20 +496,23 @@ public static Cluster createDriverCluster(ICluster<? extends IInstance> dtest)
static class IntegrationTestModule extends AbstractModule
{
private static final Logger LOGGER = LoggerFactory.getLogger(IntegrationTestModule.class);
- private final ICluster<? extends IInstance> cluster;
+ private final Iterable<? extends IInstance> instances;
Review Comment:
This is so we can customize the number of instances handled by Sidecar like we do here: https://github.com/apache/cassandra-analytics/pull/59/files#diff-a23a3d0a797ed790cbf779edbb5f6892cb31acff2d2f9bdc9c10a3347411b6d9R103
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #59:
URL: https://github.com/apache/cassandra-analytics/pull/59#discussion_r1597648858
##########
cassandra-analytics-integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/SharedClusterIntegrationTestBase.java:
##########
@@ -428,9 +436,11 @@ protected Object[][] queryAllData(QualifiedName table)
* @param consistencyLevel the consistency level to use for querying the data
* @return all the data queried from the table
*/
- protected Object[][] queryAllData(QualifiedName table, ConsistencyLevel consistencyLevel)
+ protected Object[][] queryAllData(QualifiedName table, String consistencyLevel)
Review Comment:
yep. "consolidate into a single CL type" sounds good and makes it easier to maintain in the future.
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #59:
URL: https://github.com/apache/cassandra-analytics/pull/59#discussion_r1597633049
##########
cassandra-analytics-integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/SharedClusterIntegrationTestBase.java:
##########
@@ -486,20 +496,23 @@ public static Cluster createDriverCluster(ICluster<? extends IInstance> dtest)
static class IntegrationTestModule extends AbstractModule
{
private static final Logger LOGGER = LoggerFactory.getLogger(IntegrationTestModule.class);
- private final ICluster<? extends IInstance> cluster;
+ private final Iterable<? extends IInstance> instances;
Review Comment:
The goal is to allow a Sidecar service to handle a subset of instances from the cluster. For that we don't expose the cluster itself, but rather only give the Sidecar module access to the instances it has to handle.
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh merged PR #59:
URL: https://github.com/apache/cassandra-analytics/pull/59
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
Posted by "frankgh (via GitHub)" <gi...@apache.org>.
frankgh commented on code in PR #59:
URL: https://github.com/apache/cassandra-analytics/pull/59#discussion_r1597632737
##########
cassandra-analytics-integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/SharedClusterIntegrationTestBase.java:
##########
@@ -428,9 +436,11 @@ protected Object[][] queryAllData(QualifiedName table)
* @param consistencyLevel the consistency level to use for querying the data
* @return all the data queried from the table
*/
- protected Object[][] queryAllData(QualifiedName table, ConsistencyLevel consistencyLevel)
+ protected Object[][] queryAllData(QualifiedName table, String consistencyLevel)
Review Comment:
The main reason to use String here is convenience. At `org.apache.cassandra.sidecar.testing.SharedClusterIntegrationTestBase` we use type `org.apache.cassandra.distributed.api.ConsistencyLevel`, but at the call sites we have other types available. In this case, we have type `com.datastax.driver.core.ConsistencyLevel` from the `org.apache.cassandra.analytics.TestConsistencyLevel`. Alternatively we can try to consolidate into a single CL type that we use across all of the tests. WDYT?
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #59:
URL: https://github.com/apache/cassandra-analytics/pull/59#discussion_r1597549593
##########
cassandra-analytics-integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/SharedClusterIntegrationTestBase.java:
##########
@@ -428,9 +436,11 @@ protected Object[][] queryAllData(QualifiedName table)
* @param consistencyLevel the consistency level to use for querying the data
* @return all the data queried from the table
*/
- protected Object[][] queryAllData(QualifiedName table, ConsistencyLevel consistencyLevel)
+ protected Object[][] queryAllData(QualifiedName table, String consistencyLevel)
Review Comment:
nit: prefer type over string
##########
cassandra-analytics-integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/SharedClusterIntegrationTestBase.java:
##########
@@ -486,20 +496,23 @@ public static Cluster createDriverCluster(ICluster<? extends IInstance> dtest)
static class IntegrationTestModule extends AbstractModule
{
private static final Logger LOGGER = LoggerFactory.getLogger(IntegrationTestModule.class);
- private final ICluster<? extends IInstance> cluster;
+ private final Iterable<? extends IInstance> instances;
Review Comment:
`ICluster` is already the `Iterable`. Why changing to the new type here?
Beside, `ICluster` has convenient methods to return a stream of the instances.
##########
cassandra-analytics-integration-tests/src/test/java/org/apache/cassandra/analytics/BulkWriteDownInstanceTest.java:
##########
@@ -0,0 +1,145 @@
+/*
+ * 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.analytics;
+
+import java.util.AbstractMap;
+import java.util.Arrays;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Set;
+import java.util.stream.IntStream;
+import java.util.stream.Stream;
+
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
+
+import org.apache.cassandra.distributed.api.IInstance;
+import org.apache.cassandra.sidecar.testing.QualifiedName;
+import org.apache.cassandra.spark.bulkwriter.WriterOptions;
+import org.apache.cassandra.testing.ClusterBuilderConfiguration;
+import org.apache.spark.sql.DataFrameWriter;
+import org.apache.spark.sql.Dataset;
+import org.apache.spark.sql.Row;
+import org.apache.spark.sql.SparkSession;
+
+import static com.datastax.driver.core.ConsistencyLevel.ALL;
+import static com.datastax.driver.core.ConsistencyLevel.LOCAL_QUORUM;
+import static com.datastax.driver.core.ConsistencyLevel.ONE;
+import static org.apache.cassandra.analytics.ResiliencyTestBase.uniqueTestTableFullName;
+import static org.apache.cassandra.testing.TestUtils.CREATE_TEST_TABLE_STATEMENT;
+import static org.apache.cassandra.testing.TestUtils.DC1_RF3;
+import static org.apache.cassandra.testing.TestUtils.ROW_COUNT;
+import static org.apache.cassandra.testing.TestUtils.TEST_KEYSPACE;
+
+/**
+ * Tests bulk writes in different scenarios when Cassandra instances are down
+ */
+class BulkWriteDownInstanceTest extends SharedClusterSparkIntegrationTestBase
+{
+ Set<IInstance> downInstances = new HashSet<>();
+
+ @ParameterizedTest(name = "{index} => instanceDownCount={0} {1}")
Review Comment:
nit: so it outputs `instanceDownCount=0, readCL=ONE, writeCL=ALL`
```suggestion
@ParameterizedTest(name = "{index} => instanceDownCount={0}, {1}")
```
##########
cassandra-analytics-integration-tests/src/test/java/org/apache/cassandra/analytics/replacement/HostReplacementMultiDCInsufficientReplicasTest.java:
##########
@@ -62,25 +61,10 @@ class HostReplacementMultiDCInsufficientReplicasTest extends HostReplacementTest
@Test
void nodeReplacementFailureMultiDCInsufficientNodes()
{
- Throwable thrown = catchThrowable(() ->
- bulkWriterDataFrameWriter(df, QUALIFIED_NAME)
- .option(WriterOptions.BULK_WRITER_CL.name(), EACH_QUORUM.name())
- .save());
+ DataFrameWriter<Row> dfWriter = bulkWriterDataFrameWriter(df, QUALIFIED_NAME)
+ .option(WriterOptions.BULK_WRITER_CL.name(), EACH_QUORUM.name());
- assertThat(thrown).isInstanceOf(RuntimeException.class)
- .hasMessageContaining("java.lang.RuntimeException: Bulk Write to Cassandra has failed");
-
- Throwable cause = thrown;
-
- // Find the cause
- while (cause != null && !StringUtils.contains(cause.getMessage(), "Failed to load"))
- {
- cause = cause.getCause();
- }
-
- assertThat(cause).isNotNull()
- .hasMessageFindingMatch("Failed to load (\\d+) ranges with EACH_QUORUM for " +
- "job ([a-zA-Z0-9-]+) in phase Environment Validation.");
+ sparkTestUtils.assertExpectedBulkWriteFailure(EACH_QUORUM.name(), dfWriter);
Review Comment:
👍
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org
Re: [PR] CASSANDRA-19634: Improve test coverage for downed instances [cassandra-analytics]
Posted by "yifan-c (via GitHub)" <gi...@apache.org>.
yifan-c commented on code in PR #59:
URL: https://github.com/apache/cassandra-analytics/pull/59#discussion_r1597650872
##########
cassandra-analytics-integration-framework/src/main/java/org/apache/cassandra/sidecar/testing/SharedClusterIntegrationTestBase.java:
##########
@@ -486,20 +496,23 @@ public static Cluster createDriverCluster(ICluster<? extends IInstance> dtest)
static class IntegrationTestModule extends AbstractModule
{
private static final Logger LOGGER = LoggerFactory.getLogger(IntegrationTestModule.class);
- private final ICluster<? extends IInstance> cluster;
+ private final Iterable<? extends IInstance> instances;
Review Comment:
Now I see
--
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: commits-unsubscribe@cassandra.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@cassandra.apache.org
For additional commands, e-mail: commits-help@cassandra.apache.org