aweisberg commented on code in PR #4684: URL: https://github.com/apache/cassandra/pull/4684#discussion_r2968007212
########## test/distributed/org/apache/cassandra/distributed/test/accord/AccordSingleTokenBatchTest.java: ########## @@ -0,0 +1,118 @@ +/* + * 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.test.accord; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.atomic.AtomicBoolean; + +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; +import net.bytebuddy.implementation.MethodDelegation; +import net.bytebuddy.implementation.bind.annotation.SuperCall; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.cassandra.db.Mutation; +import org.apache.cassandra.distributed.api.ConsistencyLevel; +import org.apache.cassandra.distributed.api.Feature; +import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.locator.ReplicaPlan; +import org.apache.cassandra.service.StorageProxy; +import org.apache.cassandra.transport.Dispatcher; +import org.apache.cassandra.utils.Shared; +import org.apache.cassandra.utils.TimeUUID; + +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +public class AccordSingleTokenBatchTest extends AccordTestBase +{ + private static final Logger logger = LoggerFactory.getLogger(AccordSingleTokenBatchTest.class); + + @Override + protected Logger logger() + { + return logger; + } + + @BeforeClass + public static void setupClass() throws IOException + { + AccordTestBase.setupCluster(builder -> builder + .withoutVNodes() + .withInstanceInitializer(BBHelper::install) + .withConfig(config -> + config + .with(Feature.NETWORK, Feature.GOSSIP)), 6); + } + + @Test + public void accordSinglePartitionKeyBatchTest() throws Throwable + { + List<String> ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 3}", + "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", + "CREATE TABLE " + qualifiedRegularTableName + " (k int PRIMARY KEY, v int)"); + test(ddls, cluster -> { + cluster.coordinator(1).execute("BEGIN BATCH\n" + + "INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (1, 2);\n" + + "INSERT INTO " + qualifiedRegularTableName + " (k, v) VALUES (1, 3);\n" + + "APPLY BATCH;", ConsistencyLevel.ONE); + + SimpleQueryResult r1 = cluster.coordinator(1).executeWithResult("SELECT * FROM " + qualifiedAccordTableName + " WHERE k = 1", ConsistencyLevel.ONE); + SimpleQueryResult r2 = cluster.coordinator(1).executeWithResult("SELECT * FROM " + qualifiedRegularTableName + " WHERE k = 1", ConsistencyLevel.ONE); + + assert(r1.toObjectArrays().length == 1); + assert(r2.toObjectArrays().length == 1); + + assert(State.batchLogPath.get()); + }); + } + + @Shared + public static class State + { + public static AtomicBoolean batchLogPath = new AtomicBoolean(false); + } + + public static class BBHelper Review Comment: ByteBuddy is kind of a maintenance headache because it quickly fails to match the methods it is supposed to be updating if the name or signature changes as there is no static compile time check. I almost always try to find some way to use a metric instead. In this instance I don't see a metric in `mutateAtomically` so you can inspect the batch log table itself. You might need to pause batch log delivery to make sure the entry in the batch log table isn't deleted. ########## doc/modules/cassandra/pages/architecture/cql-on-accord.adoc: ########## @@ -549,11 +549,11 @@ replay only makes a single attempt to replay before converting the batch contents to hints. If part of the batch was routed to Accord then there is no node to hint so there is a fake node that a hint is written to and when that hint is dispatched it will be split and then executed -appropriately. In https://issues.apache.org/jira/browse/CASSANDRA-20588[CASSANDRA-20588] this needs to be simplified to writing the -entire batch through Accord if any part of it should be written through -Accord because it also addresses an atomicity issue with single token -batches which can be torn when part is applied through Accord and part -is applied through Cassandra. +appropriately. Single token batches that involve both an Accord and non-Accord Review Comment: It's non-obvious what is going on here because many readers won't know what normally happens. Remove the last sentence and then update the first sentence to say that single token batch that span both Accord and non-Accord tables are written to the batch log to preserve all or nothing application. This incurs an additional performance cost because normally these single token batches would not be written to the batch log. ########## test/distributed/org/apache/cassandra/distributed/test/accord/AccordSingleTokenBatchTest.java: ########## @@ -0,0 +1,118 @@ +/* + * 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.test.accord; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.atomic.AtomicBoolean; + +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; +import net.bytebuddy.implementation.MethodDelegation; +import net.bytebuddy.implementation.bind.annotation.SuperCall; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.cassandra.db.Mutation; +import org.apache.cassandra.distributed.api.ConsistencyLevel; +import org.apache.cassandra.distributed.api.Feature; +import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.locator.ReplicaPlan; +import org.apache.cassandra.service.StorageProxy; +import org.apache.cassandra.transport.Dispatcher; +import org.apache.cassandra.utils.Shared; +import org.apache.cassandra.utils.TimeUUID; + +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +public class AccordSingleTokenBatchTest extends AccordTestBase Review Comment: It's not worth a whole new class and setting up a cluster. Can you add this to AccordCQLTestBase? ########## src/java/org/apache/cassandra/cql3/statements/BatchStatement.java: ########## @@ -541,7 +543,11 @@ private void executeWithoutConditions(List<? extends IMutation> mutations, Consi updatePartitionsPerBatchMetrics(mutations.size()); - boolean mutateAtomic = (isLogged() && mutations.size() > 1); + // We special case single token batch statements that span both Accord and non-Accord + // tables to go through the batch log in order to preserve all or nothing application + // see CASSANDRA-20588 for more details + boolean isSingleTokenBatchStatementSpanningAccordAndNonAccordTables = (mutations.size() == 1) && (containsAccordMutation(ClusterMetadata.current(), mutations.get(0))); Review Comment: I think this is being done in the wrong file. Update `StorageProxy.mutateWithConditions` to split the mutations and then use the result of the split to decide what is happening. A helper function can iterate the split mutation and look for the pattern we want to specially route to the batch log path. You can pass the already split mutations into the atomic and non-atomic mutation methods to re-use that work. The pattern to look for is split across Accord and non-Accord with more than one table present. You don't even need to have a set for tracking table UUID just remember the first one you see and if you see a different one in the other set of tables then you know it needs to go down the batch log route. I don't love asking for this because it's gong to be messy, but every mutation has to pass through here so avoiding repeating map lookups and constructing filtered mutations seems worth it. ########## test/distributed/org/apache/cassandra/distributed/test/accord/AccordSingleTokenBatchTest.java: ########## @@ -0,0 +1,118 @@ +/* + * 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.test.accord; + +import java.io.IOException; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; +import java.util.concurrent.Callable; +import java.util.concurrent.atomic.AtomicBoolean; + +import net.bytebuddy.ByteBuddy; +import net.bytebuddy.dynamic.loading.ClassLoadingStrategy; +import net.bytebuddy.implementation.MethodDelegation; +import net.bytebuddy.implementation.bind.annotation.SuperCall; + +import org.junit.BeforeClass; +import org.junit.Test; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import org.apache.cassandra.db.Mutation; +import org.apache.cassandra.distributed.api.ConsistencyLevel; +import org.apache.cassandra.distributed.api.Feature; +import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.locator.ReplicaPlan; +import org.apache.cassandra.service.StorageProxy; +import org.apache.cassandra.transport.Dispatcher; +import org.apache.cassandra.utils.Shared; +import org.apache.cassandra.utils.TimeUUID; + +import static net.bytebuddy.matcher.ElementMatchers.named; +import static net.bytebuddy.matcher.ElementMatchers.takesArguments; + +public class AccordSingleTokenBatchTest extends AccordTestBase +{ + private static final Logger logger = LoggerFactory.getLogger(AccordSingleTokenBatchTest.class); + + @Override + protected Logger logger() + { + return logger; + } + + @BeforeClass + public static void setupClass() throws IOException + { + AccordTestBase.setupCluster(builder -> builder + .withoutVNodes() + .withInstanceInitializer(BBHelper::install) + .withConfig(config -> + config + .with(Feature.NETWORK, Feature.GOSSIP)), 6); + } + + @Test + public void accordSinglePartitionKeyBatchTest() throws Throwable + { + List<String> ddls = Arrays.asList("DROP KEYSPACE IF EXISTS " + KEYSPACE + ';', + "CREATE KEYSPACE " + KEYSPACE + " WITH REPLICATION={'class':'SimpleStrategy', 'replication_factor': 3}", + "CREATE TABLE " + qualifiedAccordTableName + " (k int PRIMARY KEY, v int) WITH transactional_mode='full'", + "CREATE TABLE " + qualifiedRegularTableName + " (k int PRIMARY KEY, v int)"); + test(ddls, cluster -> { + cluster.coordinator(1).execute("BEGIN BATCH\n" + + "INSERT INTO " + qualifiedAccordTableName + " (k, v) VALUES (1, 2);\n" + + "INSERT INTO " + qualifiedRegularTableName + " (k, v) VALUES (1, 3);\n" + + "APPLY BATCH;", ConsistencyLevel.ONE); + + SimpleQueryResult r1 = cluster.coordinator(1).executeWithResult("SELECT * FROM " + qualifiedAccordTableName + " WHERE k = 1", ConsistencyLevel.ONE); + SimpleQueryResult r2 = cluster.coordinator(1).executeWithResult("SELECT * FROM " + qualifiedRegularTableName + " WHERE k = 1", ConsistencyLevel.ONE); + + assert(r1.toObjectArrays().length == 1); Review Comment: Use `assertTrue` not `assert` because `assert` can be disabled. Except everyone always runs C* and tests with assertions enabled anyways because running without assertions enabled even in production is pretty risky. When a check can be disabled it's still not an assert because there then you should add a property in CassandraRelevantProperties documenting the expensive assertion. So basically... `assert` is kind of dead and we don't use it anymore. I think junit also classifies errors from assert differently. They are errors not test condition failures which are different categories. -- 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]
