ifesdjeen commented on code in PR #3416: URL: https://github.com/apache/cassandra/pull/3416#discussion_r1680632823
########## test/distributed/org/apache/cassandra/distributed/test/topology/TopologyMixupTest.java: ########## @@ -0,0 +1,856 @@ +/* + * 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.topology; + +import java.io.IOException; +import java.io.UncheckedIOException; +import java.net.InetSocketAddress; +import java.nio.ByteBuffer; +import java.time.Duration; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.Comparator; +import java.util.EnumSet; +import java.util.HashSet; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.NoSuchElementException; +import java.util.Objects; +import java.util.Set; +import java.util.concurrent.CopyOnWriteArrayList; +import java.util.concurrent.atomic.AtomicLong; +import java.util.stream.Collectors; +import java.util.stream.Stream; + +import javax.annotation.Nullable; + +import com.google.common.base.Throwables; +import com.google.common.collect.Sets; +import org.junit.Test; + +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import accord.coordinate.Exhausted; +import accord.local.Node; +import accord.primitives.Ranges; +import accord.primitives.TxnId; +import accord.utils.Gen; +import accord.utils.Gens; +import accord.utils.Invariants; +import accord.utils.Property.Command; +import accord.utils.Property.Commands; +import accord.utils.Property.SimpleCommand; +import accord.utils.RandomSource; +import org.agrona.collections.Int2ObjectHashMap; +import org.agrona.collections.IntArrayList; +import org.agrona.collections.IntHashSet; +import org.apache.cassandra.config.CassandraRelevantProperties; +import org.apache.cassandra.config.Config; +import org.apache.cassandra.config.DatabaseDescriptor; +import org.apache.cassandra.cql3.ast.Mutation; +import org.apache.cassandra.cql3.ast.Select; +import org.apache.cassandra.cql3.ast.Statement; +import org.apache.cassandra.cql3.ast.Txn; +import org.apache.cassandra.distributed.Cluster; +import org.apache.cassandra.distributed.api.ConsistencyLevel; +import org.apache.cassandra.distributed.api.Feature; +import org.apache.cassandra.distributed.api.IInstance; +import org.apache.cassandra.distributed.api.IInvokableInstance; +import org.apache.cassandra.distributed.api.NodeToolResult; +import org.apache.cassandra.distributed.api.SimpleQueryResult; +import org.apache.cassandra.distributed.api.TokenSupplier; +import org.apache.cassandra.distributed.impl.INodeProvisionStrategy; +import org.apache.cassandra.distributed.shared.ClusterUtils; +import org.apache.cassandra.distributed.test.TestBaseImpl; +import org.apache.cassandra.distributed.test.accord.AccordTestBase; +import org.apache.cassandra.exceptions.WriteTimeoutException; +import org.apache.cassandra.io.sstable.format.SSTableFormat; +import org.apache.cassandra.locator.InetAddressAndPort; +import org.apache.cassandra.schema.ReplicationParams; +import org.apache.cassandra.schema.TableMetadata; +import org.apache.cassandra.service.accord.AccordService; +import org.apache.cassandra.service.accord.api.AccordAgent; +import org.apache.cassandra.service.consensus.TransactionalMode; +import org.apache.cassandra.tcm.ClusterMetadata; +import org.apache.cassandra.tcm.ClusterMetadataService; +import org.apache.cassandra.utils.CassandraGenerators; +import org.apache.cassandra.utils.Isolated; +import org.apache.cassandra.utils.Shared; +import org.awaitility.Awaitility; + +import static accord.utils.Property.multistep; +import static accord.utils.Property.stateful; +import static org.apache.cassandra.utils.AccordGenerators.fromQT; + +public class TopologyMixupTest extends TestBaseImpl Review Comment: > If you can point me to stuff, that would be great. Accord bootstrap is different than normal, but the streaming part is the same... so reusing shouldn't be too hard to get working for accord tables Sure, just take a look at https://github.com/apache/cassandra/blob/trunk/test/distributed/org/apache/cassandra/fuzz/ring/ConsistentBootstrapTest.java#L69-L71 In short, all you need to do is ``` ReplayingHistoryBuilder harry = HarryHelper.dataGen(new InJvmSut(cluster), new TokenPlacementModel.SimpleReplicationFactor(3), SystemUnderTest.ConsistencyLevel.ALL); for (int i = 0; i < WRITES; i++) harry.insert(); harry.validateAll(harry.quiescentLocalChecker()); ``` Quiescent local checker (https://github.com/apache/cassandra/blob/42d18a83e4468827da234c68fad3e3058b368d7c/test/harry/main/org/apache/cassandra/harry/sut/injvm/QuiescentLocalStateChecker.java#L37) will infer the placements from the current ring, and will validate that every row is where it should be. In other words, if ownership of a range has changed from node A to node B, it will ensure that the data has moved. > "get overview"? what do you mean by this? You are right, I should have expanded on this instead of making a very vague statement. Let me try to remedy this. First of all, I think the test itself is great, and the only changes I am proposing are optical / structural, and not functional. Just to make it easier for people approaching it for the first time to navigate and expand it. This class is currently at almost 900LOC. To get an overview is to be able to glance at the class, and understand what it is doing, how to extend it, or what to do when I am trying to debug an issue with the test failure. Maybe I was overly vague, so let me be more specific. I would do the following things: * Most of the methods that create `SimpleCommand` can be made into static factory methods, such as `repairCommand`, `stopInstance`. They can be moved to the top level of the class, and bunched together. This way, anyone looking at the class can see what commands it offers, and navigate to a specific command that interests them quickly. * Some code that creates `SimpleCommand` is now inside a `topologyCommand` method. I think that the method itself does make sense, but I also think that individual commands (i.e. `addNode`, `decomission`, `removeNode`, `assasinate`) can benefit from being moved to their own static methods to be near other operations. `topologyCommand` can simply call those methods * Expand some of the abbreviations. For example, I'd use `rng` instead of `rs` since it is more commonly used, and expand `th` into `topologyHistory` or just `history`. * I'd move enums such as `TopologyChange` and `RemoveType` up, since they help the person understand what's going on in the class, but auxiliary methods such as `asSet`, `executeTxn` and `maybeCreateUDTs`, since they're incidental to the implementation. With these changes, the person will be able to look at the signatures to better understand what exactly operations are already implemented, and will be able to pick them up and maybe use them in other contexts, for example for testing bootstrap, or decom, etc. Adding a new operation can then also be more straightforward: a person would simply add a new `static` method specifying the operation and then hook it up to the test quickly. Please let me know if this was helpful! -- 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]
