You are viewing a plain text version of this content. The canonical link for it is here.
Posted to pr@cassandra.apache.org by "dcapwell (via GitHub)" <gi...@apache.org> on 2023/08/29 16:57:19 UTC
[GitHub] [cassandra-accord] dcapwell opened a new pull request, #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
dcapwell opened a new pull request, #65:
URL: https://github.com/apache/cassandra-accord/pull/65
(no 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: 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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309152741
##########
accord-core/src/main/java/accord/messages/WaitUntilApplied.java:
##########
@@ -170,6 +170,7 @@ private ReadNack apply(SafeCommandStore safeStore, SafeCommand safeCommand)
}
else
{
+ // TODO (debug): this will cause us to send a Nack back, so why are we listening?
Review Comment:
will remove, but in debugger I wasn't sure why we do this... `NotCommitted` causes the process to return a `Nack` which causes the waiting to halt! In my usage I keep retrying the wait over and over again when a `NotCommitted` or a `Timeout` is observed...
##########
accord-core/src/main/java/accord/topology/Topology.java:
##########
@@ -147,11 +179,6 @@ public boolean isSubset()
return supersetIndexes.length < shards.length;
}
- public Topology withEpoch(long epoch)
Review Comment:
moved to `TopologyUtils` under `src/test` as the semantics are not clear what to do here... *should* this also update the `global` reference? Since this was only used by tests and only for the `global` state anyways felt was easier to punt figuring out the semantic and just make it a test method
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408557584
##########
accord-core/src/test/java/accord/Utils.java:
##########
@@ -118,6 +123,16 @@ public static Txn writeTxn(Ranges ranges)
return new Txn.InMemory(ranges, MockStore.read(ranges), MockStore.QUERY, MockStore.update(ranges));
}
+ public static Txn listWriteTxn(Node.Id client, Keys keys)
+ {
+ ListUpdate update = new ListUpdate(Function.identity());
+ for (Key k : keys)
+ update.put(k, 1);
Review Comment:
this isn't 100% correct... if you run this txn twice the second one will freak out as ListRequest won't like this... it works just fine for single run cases (which is what I do)... I guess I could add a `int...` or something to allow overriding... but didn't want to complicate things too much
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443158738
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
looking into this now
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1391140707
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
> unreplicated keys from the route to prevent invalidate
Can you say more? This should include all keys/ranges for the txn
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309150411
##########
accord-core/src/main/java/accord/impl/SimpleProgressLog.java:
##########
@@ -277,7 +277,7 @@ void run(SafeCommandStore safeStore, SafeCommand safeCommand)
updateMax(success.asProgressToken());
}
- });
+ }).begin(node.agent());
Review Comment:
the leak detector found this case. `CommandStore` returns a `AsyncChain` and not `AsyncResult`! In Cassandra we use `AsyncChains.Head` so require `begin` to start, but in the in-memory stories we returned a `AsyncResult` which would make code work under tests but not in C*...
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309175076
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -357,6 +420,7 @@ public static void main(String[] args)
@Test
public void testOne()
{
+// run(726880888609333L, 1000); // taking a long time... gave up after 15 minutes!
Review Comment:
will revert, this is just a seed that hit a while true loop in recovery. Spoke to Benedict and seems that his branch may fix this
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -311,6 +373,7 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
logger.info("Received {} acks, {} nacks, {} lost, {} truncated ({} total) to {} operations", acks.get(), nacks.get(), lost.get(), truncated.get(), acks.get() + nacks.get() + lost.get() + truncated.get(), operations);
logger.info("Message counts: {}", messageStatsMap.entrySet());
+ logger.info("Took {} and in logical time of {}", Duration.ofNanos(System.nanoTime() - startNanos), Duration.ofMillis(queue.nowInMillis() - startLogicalMillis));
Review Comment:
this was mostly for me... I wanted to get a better grasp of how much "logical" time has passed as there is a lot of delayed logic
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309219019
##########
accord-core/src/test/java/accord/impl/list/ListStore.java:
##########
@@ -96,4 +328,222 @@ private static boolean isStrictPrefix(int[] a, int[] b)
}
return true;
}
+
+ public synchronized void onTopologyUpdate(Node node, Topology topology)
+ {
+ long epoch = topology.epoch();
+ Ranges updatedRanges = topology.rangesForNode(node.id());
+ if (previousTopology == null)
+ {
+ previousTopology = topology;
+ allowed = updatedRanges;
+ addedAts.add(new ChangeAt(epoch, updatedRanges));
+ return;
+ }
+
+ Ranges previousRanges = previousTopology.rangesForNode(node.id());
+
+ Ranges added = updatedRanges.subtract(previousRanges);
+ Ranges removed = previousRanges.subtract(updatedRanges);
+ if (!added.isEmpty())
+ {
+ addedAts.add(new ChangeAt(epoch, added));
+ for (Range range : added)
+ {
+ if (!previousTopology.ranges().intersects(range))
+ {
+ // A range was added that globally didn't exist before; there is nothing to bootstrap here!
+ // TODO (now): document this history change
+ allowed = allowed.with(Ranges.of(range));
+ }
+ }
+ }
+ if (!removed.isEmpty())
+ {
+ pendingRemoves.add(epoch);
+ removedAts.add(new ChangeAt(epoch, removed));
+ // There are 2 different types of remove
+ // 1) node no longer covers, but the range exists in the cluster
+ // 2) the range no longer exists in the cluster
+ // Given this, we need 2 different solutions for the purge logic as sync points are not safe when running in older epoch.
+ Ranges localRemove = Ranges.EMPTY;
+ Ranges globalRemove = Ranges.EMPTY;
+ for (Range range : removed)
+ {
+ if (topology.ranges().intersects(range)) localRemove = localRemove.with(Ranges.of(range));
+ else globalRemove = globalRemove.with(Ranges.of(range));
+ }
+ if (!localRemove.isEmpty())
+ {
+ Ranges finalLocalRemove = localRemove;
+ runWhenReady(node, epoch, () -> removeLocalWhenReady(node, epoch, finalLocalRemove));
+ }
+ // TODO (correctness, coverage): add cleanup logic for global ranges removed. This must be solved in CASSANDRA-18675
+ }
+ previousTopology = topology;
+ }
+
+ private void runWhenReady(Node node, long epoch, Runnable whenKnown)
+ {
+ if (node.topology().epoch() >= epoch) whenKnown.run();
Review Comment:
was finding issues as this logic happens *before* accord sees the epoch (to better mimic TCM) and the wait logic didn't expect gaps... so implemented my own wait logic...
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443482838
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
Review Comment:
I went with `txnGenerator`, not sure a better name... it generates `Txn`
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#issuecomment-1883871947
Here are `BurnTest` seeds that fail for me (in C* IntelliJ) that I feel are not caused by this patch
```
Suppressed: java.lang.IllegalStateException: Unexpected status for Command@1920466142{[12,29534,1(RR),1]:ReadyToExecute}
at accord.utils.Invariants.illegalState(Invariants.java:44)
at accord.utils.Invariants.checkState(Invariants.java:127)
at accord.coordinate.Infer$InvalidateAndCallback.apply(Infer.java:265)
at accord.coordinate.Infer$CleanupAndCallback.apply(Infer.java:229)
at accord.coordinate.Infer$CleanupAndCallback.apply(Infer.java:201)
*/
run(4016471194711250L, 1000);
```
```
/*
Suppressed: java.lang.IllegalStateException: [1, 2, 3, 4] != [1, 2, 3]
at accord.utils.Invariants.illegalState(Invariants.java:44)
at accord.utils.Invariants.checkState(Invariants.java:133)
at accord.utils.Timestamped.mergeEqual(Timestamped.java:67)
at accord.impl.list.ListData.lambda$merge$0(ListData.java:37)
at java.base/java.util.Map.merge(Map.java:1270)
at accord.impl.list.ListData.lambda$merge$1(ListData.java:37)
at java.base/java.util.TreeMap.forEach(TreeMap.java:1002)
at accord.impl.list.ListData.merge(ListData.java:36)
at accord.coordinate.TxnExecute.process(TxnExecute.java:104)
at accord.coordinate.TxnExecute.process(TxnExecute.java:49)
at accord.coordinate.ReadCoordinator.onSuccess(ReadCoordinator.java:121)
at accord.coordinate.ReadCoordinator.onSuccess(ReadCoordinator.java:40)
*/
run(4016642298723958L, 1000);
```
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1411330028
##########
accord-core/src/test/java/accord/Utils.java:
##########
@@ -118,6 +123,16 @@ public static Txn writeTxn(Ranges ranges)
return new Txn.InMemory(ranges, MockStore.read(ranges), MockStore.QUERY, MockStore.update(ranges));
}
+ public static Txn listWriteTxn(Node.Id client, Keys keys)
+ {
+ ListUpdate update = new ListUpdate(Function.identity());
+ for (Key k : keys)
+ update.put(k, 1);
Review Comment:
that's fine. If it has sharp edges that make it unsuitable outside of a narrow use case though, it shouldn't go in Utils.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1411327166
##########
accord-core/src/test/java/accord/Utils.java:
##########
@@ -118,6 +123,16 @@ public static Txn writeTxn(Ranges ranges)
return new Txn.InMemory(ranges, MockStore.read(ranges), MockStore.QUERY, MockStore.update(ranges));
}
+ public static Txn listWriteTxn(Node.Id client, Keys keys)
+ {
+ ListUpdate update = new ListUpdate(Function.identity());
+ for (Key k : keys)
+ update.put(k, 1);
Review Comment:
possible... I really didn't bother testing... I think `Timestamped` doesn't like this... its been awhile...
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1411399776
##########
accord-core/src/test/java/accord/Utils.java:
##########
@@ -118,6 +123,16 @@ public static Txn writeTxn(Ranges ranges)
return new Txn.InMemory(ranges, MockStore.read(ranges), MockStore.QUERY, MockStore.update(ranges));
}
+ public static Txn listWriteTxn(Node.Id client, Keys keys)
+ {
+ ListUpdate update = new ListUpdate(Function.identity());
+ for (Key k : keys)
+ update.put(k, 1);
Review Comment:
tested, we do just append, so this is actually safe...
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1391242710
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
I'll try to understand `commandStore.canExposeUnloaded()` and get back to you. This looks to be a store property and not based off the requested data...
I added the cache logic as I noticed that the Cassandra logic was drastically different than InMemory logic, and we kept having logic optimized for InMemory logic... so I wanted to get the 2 stores closer as majority of testing is in BurnTest and far less is done with Cassandra...
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442886320
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
Review Comment:
more descriptive name?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442864172
##########
accord-core/src/main/java/accord/impl/CommandTimeseries.java:
##########
@@ -128,14 +128,17 @@ public Timestamp minTimestamp()
Timestamp result = null;
for (D data : commands.values())
{
+ TxnId txnId = loader.txnId(data);
+ Timestamp executeAt = loader.executeAt(data);
if (result == null)
{
- result = Timestamp.min(loader.txnId(data), loader.executeAt(data));
+ result = executeAt == null ? txnId : Timestamp.min(txnId, executeAt);
Review Comment:
`Timestamp.nonNullOrMin`?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1445966135
##########
accord-core/src/main/java/accord/primitives/AbstractRanges.java:
##########
@@ -56,7 +56,10 @@ public int indexOf(RoutableKey key)
@Override
public int indexOf(Range find)
Review Comment:
I think it would be cleaner to either call it `ceilIndexOf` or just supply/accept `CEIL` parameter, so we don't mix descriptors of functionality.
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309213454
##########
accord-core/src/test/java/accord/impl/list/ListStore.java:
##########
@@ -49,28 +147,162 @@ public ListStore(Node.Id node)
this.node = node;
}
- public Timestamped<int[]> get(Key key)
+ public synchronized Timestamped<int[]> get(Ranges unavailable, Timestamp executeAt, Key key)
Review Comment:
`synchronized` was from a different patch and only kept it here as `write` had it...
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1323351402
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
We have a few of places that try to access data outside of the `PreLoadContext` and acts on the global cache.
`accord.impl.InMemoryCommandStore#ifPresent(accord.primitives.TxnId)`
`accord.impl.InMemoryCommandStore.InMemorySafeStore#getIfLoaded(accord.primitives.TxnId)`
`accord.impl.AbstractSafeCommandStore#getInternalIfLoadedAndInitialised`
`accord.local.SafeCommandStore#ifLoadedAndInitialised`
`accord.local.Commands.NotifyWaitingOn#accept`
```
SafeCommand curSafe = ifLoadedAndInitialised(safeStore, depth);
Command cur = curSafe != null ? curSafe.current() : null;
if (cur == null)
{
// need to load; schedule execution for later
safeStore.commandStore().execute(this, this).begin(safeStore.agent());
return;
}
```
Cases like this would be 100% cache hit in BurnTest, so the non-cache hit case was never tested.
EDIT: `s/a lot/a few/g`
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
s/a lot/a few/g
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1387295720
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
can you take a look at how we handled this in the prune deps patch? Instead of adding an actual cache, there's a check to determine if we can expose objects not specified in the preload context. Then we can have it return true/false based on some random distribution. Aside from having a lower footprint, I think it would simulate a wider variety of eviction patterns.
See https://github.com/bdeggleston/cassandra-accord/blob/C18784-reduce-deps-v2/accord-core/src/main/java/accord/impl/InMemoryCommandStore.java#L759C51-L759C51
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
wouldn't we want to exclude the now unreplicated keys from the route to prevent invalidate from running into mismatch issues itself?
##########
accord-core/src/main/java/accord/coordinate/TopologyMismatch.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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 accord.coordinate;
+
+import java.util.EnumSet;
+
+import javax.annotation.Nullable;
+
+import accord.api.RoutingKey;
+import accord.primitives.Routables;
+import accord.primitives.Seekables;
+import accord.primitives.TxnId;
+import accord.topology.Topology;
+
+public class TopologyMismatch extends CoordinationFailed
+{
+ public final Topology topology;
+ public final Seekables<?, ?> keysOrRanges;
+ private final EnumSet<Reason> reasons;
+
+ public enum Reason { HOME_KEY, KEYS_OR_RANGES }
+
+ private TopologyMismatch(EnumSet<Reason> reasons, Topology topology, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ super(txnId, homeKey, buildMessage(reasons, topology, homeKey, keysOrRanges));
+ this.reasons = reasons;
+ this.topology = topology;
+ this.keysOrRanges = keysOrRanges;
+ }
+
+ private static String buildMessage(EnumSet<Reason> reason, Topology topology, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ StringBuilder sb = new StringBuilder();
+ if (reason.contains(Reason.KEYS_OR_RANGES))
+ sb.append(String.format("Txn attempted to access keys or ranges %s that are not longer valid globally (%d -> %s)", keysOrRanges, topology.epoch(), topology.ranges()));
+ if (reason.contains(Reason.HOME_KEY))
+ {
+ if (sb.length() != 0)
+ sb.append('\n');
+ sb.append(String.format("HomeKey %s exists for a range that is no longer globally valid (%d -> %s)", homeKey, topology.epoch(), topology.ranges()));
+ }
+ return sb.toString();
+ }
+
+ @Nullable
+ public static TopologyMismatch checkForMismatch(Topology t, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ EnumSet<TopologyMismatch.Reason> reasons = null;
+ if (!t.ranges().contains(homeKey))
+ {
+ if (reasons == null)
+ reasons = EnumSet.noneOf(TopologyMismatch.Reason.class);
+ reasons.add(TopologyMismatch.Reason.HOME_KEY);
+ }
+ Seekables<?, ?> updatedRanges = keysOrRanges.slice(t.ranges(), Routables.Slice.Minimal);
+ if (!updatedRanges.containsAll(keysOrRanges))
Review Comment:
Could we use AbstractKeys#any here, or something similar so we don't have to allocate a slice?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408562427
##########
accord-core/src/test/java/accord/impl/basic/Cluster.java:
##########
@@ -496,7 +506,7 @@ private Function<List<Id>, BiFunction<Id, Id, Link>> overrideLinks(RandomSource
case RANDOM_BIDIRECTIONAL:
case RANDOM_UNIDIRECTIONAL:
boolean bidirectional = kind == RANDOM_BIDIRECTIONAL;
- int count = random.nextInt(bidirectional || random.nextBoolean() ? nodesList.size() : (nodesList.size() * nodesList.size())/2);
+ int count = random.nextInt(bidirectional || random.nextBoolean() ? nodesList.size() : Math.max(1, (nodesList.size() * nodesList.size())/2));
Review Comment:
when nodesList size = 1, this fails (`1/2 = 0), hence the `Math.max(1` to keep it safe
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408553488
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
Take a look at `accord.local.CommandsTest#removeRangesValidate` to see that this works just fine
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442889538
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
+ int[] keys = new int[keyCount];
+ {
+ IntHashSet seen = new IntHashSet();
+ for (int i = 0; i < keyCount; i++)
+ {
+ int hash;
+ do
+ {
+ // start is exclusive, and end is inclusive; which is the oposite of nextInt, so +1 to account for this
+ hash = 1 + random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ }
+ while (!seen.add(hash));
+ keys[i] = CRCUtils.reverseCRC32LittleEnding(hash);
Review Comment:
What is it we're achieving here, with this refactor? This appears to give up the possibility of hash collisions for reverse lookups, but what value do we derive from reverse lookups?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442864434
##########
accord-core/src/main/java/accord/impl/CommandTimeseries.java:
##########
@@ -128,14 +128,17 @@ public Timestamp minTimestamp()
Timestamp result = null;
for (D data : commands.values())
{
+ TxnId txnId = loader.txnId(data);
+ Timestamp executeAt = loader.executeAt(data);
if (result == null)
{
- result = Timestamp.min(loader.txnId(data), loader.executeAt(data));
+ result = executeAt == null ? txnId : Timestamp.min(txnId, executeAt);
}
else
{
- result = Timestamp.min(result, loader.txnId(data));
- result = Timestamp.min(result, loader.executeAt(data));
+ result = Timestamp.min(result, txnId);
+ if (executeAt != null)
+ result = Timestamp.min(result, executeAt);
Review Comment:
`Timestamp.nonNullOrMin`?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443370064
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
if (isRangeQuery)
{
- int rangeCount = 1 + random.nextInt(2);
- List<Range> requestRanges = new ArrayList<>();
- while (--rangeCount >= 0)
- {
- int j = 1 + random.nextInt(0xffff), i = Math.max(0, j - (1 + random.nextInt(0x1ffe)));
- requestRanges.add(IntHashKey.range(forHash(i), forHash(j)));
- }
- Ranges ranges = Ranges.of(requestRanges.toArray(new Range[0]));
- ListRead read = new ListRead(random.decide(readInCommandStore) ? Function.identity() : executor, ranges, ranges);
- ListQuery query = new ListQuery(client, count);
- ListRequest request = new ListRequest(new Txn.InMemory(ranges, read, query, null), listener);
- packets.add(new Packet(client, node, count, request));
+ description = "range";
+ gen = n -> {
+ int[] prefixes = prefixes(n.topology().current());
+
+ int rangeCount = 1 + random.nextInt(2);
+ List<Range> requestRanges = new ArrayList<>();
+ while (--rangeCount >= 0)
+ {
+ int prefix = random.pickInt(prefixes);
+ int i = random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ int j = 1 + random.nextInt(i, HASH_RANGE_END);
Review Comment:
pushed a change that I hope solves this problem. I bound ranges to be of the following sizes
```
[655, 1310, 2620, 5240, 10480, 20960] // if 20960 is too big its a config change to shrink
```
`655 = (HASH_RANGE_END - HASH_RANGE_START + 1) / 100`
and the rest of the values are `* 2` the previous.
I added a new `pickZipf` function to select the range size and made sure this logic respects it.
I looked closer at the previous logic (on trunk) and it selects from the full range of values (64k), then again from 8k, and if this wraps around we default to `0`. This was effectively selecting a range of 1-8k uniformly (not exactly, simplifying and ignoring wrap around case); figured changing the distribution away from uniform would also be a welcome 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: 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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1391143381
##########
accord-core/src/main/java/accord/coordinate/TopologyMismatch.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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 accord.coordinate;
+
+import java.util.EnumSet;
+
+import javax.annotation.Nullable;
+
+import accord.api.RoutingKey;
+import accord.primitives.Routables;
+import accord.primitives.Seekables;
+import accord.primitives.TxnId;
+import accord.topology.Topology;
+
+public class TopologyMismatch extends CoordinationFailed
+{
+ public final Topology topology;
+ public final Seekables<?, ?> keysOrRanges;
+ private final EnumSet<Reason> reasons;
+
+ public enum Reason { HOME_KEY, KEYS_OR_RANGES }
+
+ private TopologyMismatch(EnumSet<Reason> reasons, Topology topology, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ super(txnId, homeKey, buildMessage(reasons, topology, homeKey, keysOrRanges));
+ this.reasons = reasons;
+ this.topology = topology;
+ this.keysOrRanges = keysOrRanges;
+ }
+
+ private static String buildMessage(EnumSet<Reason> reason, Topology topology, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ StringBuilder sb = new StringBuilder();
+ if (reason.contains(Reason.KEYS_OR_RANGES))
+ sb.append(String.format("Txn attempted to access keys or ranges %s that are not longer valid globally (%d -> %s)", keysOrRanges, topology.epoch(), topology.ranges()));
+ if (reason.contains(Reason.HOME_KEY))
+ {
+ if (sb.length() != 0)
+ sb.append('\n');
+ sb.append(String.format("HomeKey %s exists for a range that is no longer globally valid (%d -> %s)", homeKey, topology.epoch(), topology.ranges()));
+ }
+ return sb.toString();
+ }
+
+ @Nullable
+ public static TopologyMismatch checkForMismatch(Topology t, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ EnumSet<TopologyMismatch.Reason> reasons = null;
+ if (!t.ranges().contains(homeKey))
+ {
+ if (reasons == null)
+ reasons = EnumSet.noneOf(TopologyMismatch.Reason.class);
+ reasons.add(TopologyMismatch.Reason.HOME_KEY);
+ }
+ Seekables<?, ?> updatedRanges = keysOrRanges.slice(t.ranges(), Routables.Slice.Minimal);
+ if (!updatedRanges.containsAll(keysOrRanges))
Review Comment:
I think we need `t.contains(keysOrRanges)` to avoid allocation, slice/intersect may have non-intersect which would mean incompatible change; ill try to look closer to see if we can avoid allocation here
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1445970964
##########
accord-core/src/main/java/accord/coordinate/TxnExecute.java:
##########
@@ -74,8 +75,9 @@ private TxnExecute(Node node, TxnId txnId, Txn txn, FullRoute<?> route, Particip
}
@Override
- protected void start(Set<Id> readSet)
+ protected void start(Iterable<Id> to)
{
+ Set<Id> readSet = Sets.newHashSet(to);
Review Comment:
maybe switch to `IntHashSet` and accept it in `Commit`?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1446670532
##########
accord-core/src/main/java/accord/primitives/AbstractRanges.java:
##########
@@ -56,7 +56,10 @@ public int indexOf(RoutableKey key)
@Override
public int indexOf(Range find)
Review Comment:
I used `firstIndexOf` to be similar to `java.util.List#lastIndexOf`, but switched to `ceilIndexOf` for now
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443388569
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
if (isRangeQuery)
{
- int rangeCount = 1 + random.nextInt(2);
- List<Range> requestRanges = new ArrayList<>();
- while (--rangeCount >= 0)
- {
- int j = 1 + random.nextInt(0xffff), i = Math.max(0, j - (1 + random.nextInt(0x1ffe)));
- requestRanges.add(IntHashKey.range(forHash(i), forHash(j)));
- }
- Ranges ranges = Ranges.of(requestRanges.toArray(new Range[0]));
- ListRead read = new ListRead(random.decide(readInCommandStore) ? Function.identity() : executor, ranges, ranges);
- ListQuery query = new ListQuery(client, count);
- ListRequest request = new ListRequest(new Txn.InMemory(ranges, read, query, null), listener);
- packets.add(new Packet(client, node, count, request));
+ description = "range";
+ gen = n -> {
+ int[] prefixes = prefixes(n.topology().current());
+
+ int rangeCount = 1 + random.nextInt(2);
+ List<Range> requestRanges = new ArrayList<>();
+ while (--rangeCount >= 0)
+ {
+ int prefix = random.pickInt(prefixes);
+ int i = random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ int j = 1 + random.nextInt(i, HASH_RANGE_END);
Review Comment:
my main thinking was to add a bias either to small or large ranges (I pushed bias towards small)
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442888092
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
Review Comment:
Why is that a bug? Multiple keys can map to a single hash. It should be fine to run with a hash range of [0..1) an 1000 keys if we want.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1391257698
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
ill need to look closer at this change, after rebase we are failing and looks like its a cache hit issue... looking at the patch now (a long time remove from when I wrote it) I see we do the following
```
private static <K, V> V getIfLoaded(K key, Function<K, V> get, Consumer<V> add, Function<K, V> getIfLoaded)
{
V value = get.apply(key);
if (value != null)
return value;
value = getIfLoaded.apply(key);
if (value == null)
return null;
add.accept(value);
return value;
}
```
where `get` is defined as
```
protected InMemorySafeCommand getCommandInternal(TxnId txnId)
{
return commands.get(txnId);
}
```
and `getIfLoaded` is
```
protected InMemorySafeCommand getIfLoaded(TxnId txnId)
{
GlobalCommand global = commandStore.ifPresent(txnId);
return global != null ? global.createSafeReference() : null;
}
```
so the `get` logic just reaches into the cache... so the `getIfLoaded` only happens iff `get` returned `null`... but that is just
```
public GlobalCommand ifPresent(TxnId txnId)
{
return cacheHit(txnId) ? commands.get(txnId) : null;
}
```
so not sure how that matters at all...
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1411379524
##########
accord-core/src/test/java/accord/Utils.java:
##########
@@ -118,6 +123,16 @@ public static Txn writeTxn(Ranges ranges)
return new Txn.InMemory(ranges, MockStore.read(ranges), MockStore.QUERY, MockStore.update(ranges));
}
+ public static Txn listWriteTxn(Node.Id client, Keys keys)
+ {
+ ListUpdate update = new ListUpdate(Function.identity());
+ for (Key k : keys)
+ update.put(k, 1);
Review Comment:
ill test and push this to the test if it has such a sharp edge (think it does)
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1411326316
##########
accord-core/src/test/java/accord/Utils.java:
##########
@@ -118,6 +123,16 @@ public static Txn writeTxn(Ranges ranges)
return new Txn.InMemory(ranges, MockStore.read(ranges), MockStore.QUERY, MockStore.update(ranges));
}
+ public static Txn listWriteTxn(Node.Id client, Keys keys)
+ {
+ ListUpdate update = new ListUpdate(Function.identity());
+ for (Key k : keys)
+ update.put(k, 1);
Review Comment:
I don't understand what the problem is, wouldn't ListUpdate just append another 1 if this was run twice?
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309149238
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -871,31 +903,41 @@ public static class Synchronized extends InMemoryCommandStore
{
Runnable active = null;
final Queue<Runnable> queue = new ConcurrentLinkedQueue<>();
+ private final AtomicBoolean running = new AtomicBoolean(false);
public Synchronized(int id, NodeTimeService time, Agent agent, DataStore store, ProgressLog.Factory progressLogFactory, EpochUpdateHolder epochUpdateHolder)
{
super(id, time, agent, store, progressLogFactory, epochUpdateHolder);
}
- private synchronized void maybeRun()
+ private void maybeRun()
{
- if (active != null)
+ if (!running.compareAndSet(false, true))
return;
-
- active = queue.poll();
- while (active != null)
+ synchronized (this)
Review Comment:
was not 100% sure this was fully needed given the single threaded nature of the work, but felt best to not try to change the locking too much
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309151457
##########
accord-core/src/main/java/accord/messages/TxnRequest.java:
##########
@@ -234,7 +234,7 @@ public static <I, O> O computeScope(Node.Id node, Topologies topologies, I keys,
O scope = null;
for (int i = startIndex, mi = topologies.size() ; i < mi ; ++i)
{
- Topology topology = topologies.get(i);
+ Topology topology = topologies.get(i).global();
Review Comment:
this was a bug found with remove prefix. The subset topologies would leak the `global` values and not the subset values, which caused the scope logic to work as it works off `global`, so once this was fixed I had to be explicit and ask for the `global` values...
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309146831
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -408,63 +464,39 @@ void update(Ranges add)
}
}
- static class CFKEntry extends TxnId
+ enum Loader implements CommandLoader<Command>
Review Comment:
CFK is supposed to be immutable, but the old `CFKEntry` used the global backing commands map and would return updated values, making CFK mutable! Switched away from mutable loader to immutable loader to better match the expected behavior and what C* does...
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309212177
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -354,6 +418,7 @@ public static void main(String[] args)
}
}
+ @Timeout(value = 3, unit = TimeUnit.MINUTES)
Review Comment:
this is so the test times out rather than the build... when while true loops are found the build times out due to no progress... felt it was best to have the test timeout...
One issue with this is that the seed will be lost... we don't depend on `org.awaitility` like C* does, so could also add that test dependency and update the `run(seed)` method to do this timeout and log the seed on failure.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408564257
##########
buildSrc/src/main/groovy/accord.java-conventions.gradle:
##########
@@ -49,10 +49,6 @@ test {
jvmArgs += ['-XX:+HeapDumpOnOutOfMemoryError', "-XX:HeapDumpPath=${buildDir}"]
}
-tasks.withType(Test) {
Review Comment:
looks like rebase issue that got merged... I added this, Benedict rebased onto my branch and added similar logic (see line 49), but didn't detect the logical conflict so this got merged.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442871593
##########
accord-core/src/main/java/accord/messages/Commit.java:
##########
@@ -275,6 +275,22 @@ public static void commitInvalidate(Node node, TxnId txnId, Unseekables<?> infor
// TODO (expected, safety): this kind of check needs to be inserted in all equivalent methods
Invariants.checkState(untilEpoch >= txnId.epoch());
Invariants.checkState(node.topology().hasEpoch(untilEpoch));
+ // Possible that the invalidation is due to the range no longer existing in untilEpoch, which means that
Review Comment:
So a couple of thoughts here:
1) How can this happen? We should only invoke with an `untilEpoch` we attempted to `Accept` with, and we shouldn't attempt to `Accept` in an epoch where the keys don't exist
2) If we have somehow done this, I think it is fine to contact `untilEpoch` even if some keys are missing? The problem with going to the earliest epoch we can fine that contains them is that if we have somehow gone into a future epoch, we might have left traces we should ideally invalidate for other keys that did still exist, and have moved replica
My preference would be to first figure out (1), as if it's happening it may indicate some other issue that we can address. Then we might anyway want to support a version of `preciseEpochs` that permits topology mismatches.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442865404
##########
accord-core/src/main/java/accord/local/Node.java:
##########
@@ -479,9 +486,19 @@ public <T> void send(Collection<Id> to, Function<Id, Request> requestFactory, Ca
public <T> void send(Collection<Id> to, Function<Id, Request> requestFactory, AgentExecutor executor, Callback<T> callback)
{
checkStore(executor);
+ checkIterationSafe(to);
to.forEach(dst -> messageSink.send(dst, requestFactory.apply(dst), executor, callback));
}
+ private static void checkIterationSafe(Collection<?> collection)
+ {
+ if (!Invariants.isParanoid())
+ return;
+ if (collection instanceof List) return;
+ if (collection instanceof NavigableSet || collection instanceof LinkedHashSet || "java.util.LinkedHashMap.LinkedKeySet".equals(collection.getClass().getCanonicalName())) return;
Review Comment:
`|| collection instanceof DeterministicSet`
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442885508
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
if (isRangeQuery)
{
- int rangeCount = 1 + random.nextInt(2);
- List<Range> requestRanges = new ArrayList<>();
- while (--rangeCount >= 0)
- {
- int j = 1 + random.nextInt(0xffff), i = Math.max(0, j - (1 + random.nextInt(0x1ffe)));
- requestRanges.add(IntHashKey.range(forHash(i), forHash(j)));
- }
- Ranges ranges = Ranges.of(requestRanges.toArray(new Range[0]));
- ListRead read = new ListRead(random.decide(readInCommandStore) ? Function.identity() : executor, ranges, ranges);
- ListQuery query = new ListQuery(client, count);
- ListRequest request = new ListRequest(new Txn.InMemory(ranges, read, query, null), listener);
- packets.add(new Packet(client, node, count, request));
+ description = "range";
+ gen = n -> {
+ int[] prefixes = prefixes(n.topology().current());
+
+ int rangeCount = 1 + random.nextInt(2);
+ List<Range> requestRanges = new ArrayList<>();
+ while (--rangeCount >= 0)
+ {
+ int prefix = random.pickInt(prefixes);
+ int i = random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ int j = 1 + random.nextInt(i, HASH_RANGE_END);
Review Comment:
This doesn't cap the size of the range involved in the transaction, so we'll tend towards transactions that cover half the cluster. The prior code wasn't perfect, of course - ideally we would probably vary the maximum range size for each run.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443508483
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
Review Comment:
`BurnTest` now owns the hash function, so able to remove this check.
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309155743
##########
accord-core/src/main/java/accord/topology/TopologyManager.java:
##########
@@ -498,7 +508,7 @@ public Topologies withUnsyncedEpochs(Unseekables<?> select, long minEpoch, long
return withSufficientEpochs(select, minEpoch, maxEpoch, epochState -> epochState.syncComplete);
}
- public Topologies withOpenEpochs(Unseekables<?> select, Timestamp min, Timestamp max)
+ public Topologies withOpenEpochs(Unseekables<?> select, EpochSupplier min, EpochSupplier max)
Review Comment:
this was updated for `ListStore` as it works with historic epochs; so rather than define a full `Timestamp`, using `EpochSupplier` allowed `() -> epoch` as well in this case
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309154865
##########
accord-core/src/main/java/accord/topology/TopologyManager.java:
##########
@@ -180,6 +182,14 @@ boolean syncCompleteFor(Unseekables<?> intersect)
{
return syncComplete.containsAll(intersect);
}
+
+ @Override
+ public String toString()
+ {
+ return "EpochState{" +
Review Comment:
I don't remember if this is logged, but im always in a debugger so this makes my life easier
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309198139
##########
accord-core/src/test/java/accord/topology/TopologyUtils.java:
##########
@@ -0,0 +1,191 @@
+/*
+ * 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 accord.topology;
+
+import accord.api.RoutingKey;
+import accord.impl.IntKey;
+import accord.impl.PrefixedIntHashKey;
+import accord.primitives.Range;
+import accord.local.Node;
+import accord.primitives.Ranges;
+import accord.primitives.RoutingKeys;
+import accord.primitives.Unseekables;
+import accord.primitives.Unseekables.UnseekablesKind;
+import accord.utils.Gens;
+import accord.utils.RandomSource;
+import accord.utils.Utils;
+import accord.utils.WrapAroundList;
+import accord.utils.WrapAroundSet;
+
+import javax.annotation.Nullable;
+import java.util.*;
+
+import static accord.utils.Utils.toArray;
Review Comment:
this was moved into `accord.topology` package to get access to package private state (for `withEpoch`); most of the class is not new but git/GH doesn't seem to detect the move
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309184802
##########
accord-core/src/test/java/accord/impl/basic/DelayedCommandStores.java:
##########
@@ -52,6 +59,28 @@ public static CommandStores.Factory factory(PendingQueue pending)
new DelayedCommandStores(time, agent, store, random, shardDistributor, progressLogFactory, new SimulatedDelayedExecutorService(pending, agent, random));
}
+ @Override
+ protected boolean shouldBootstrap(Node node, Topology previous, Topology updated, Range range)
+ {
+ if (!super.shouldBootstrap(node, previous, updated, range))
+ return false;
+ if (!(range.start() instanceof PrefixedIntHashKey)) return true;
Review Comment:
I think this logic could be rewritten to `previous.ranges.containsAll(Ranges.of(range))` which should actually match the C* logic... can switch but wasn't sure if there would be edge cases where we don't want that as the default behavior...
--
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
[GitHub] [cassandra-accord] bdeggleston commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1319184875
##########
accord-core/src/main/java/accord/coordinate/TopologyMismatch.java:
##########
@@ -0,0 +1,77 @@
+/*
+ * 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 accord.coordinate;
+
+import java.util.EnumSet;
+
+import javax.annotation.Nullable;
+
+import accord.api.RoutingKey;
+import accord.primitives.Routables;
+import accord.primitives.Seekables;
+import accord.primitives.TxnId;
+import accord.topology.Topology;
+
+public class TopologyMismatch extends CoordinationFailed
+{
+ public final Topology topology;
+ public final Seekables<?, ?> keysOrRanges;
+ private final EnumSet<Reason> reasons;
+
+ public enum Reason { HOME_KEY, KEYS_OR_RANGES }
+
+ private TopologyMismatch(EnumSet<Reason> reasons, Topology topology, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ super(txnId, homeKey, buildMessage(reasons, topology, homeKey, keysOrRanges));
+ this.reasons = reasons;
+ this.topology = topology;
+ this.keysOrRanges = keysOrRanges;
+ }
+
+ private static String buildMessage(EnumSet<Reason> reason, Topology topology, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ StringBuilder sb = new StringBuilder();
+ if (reason.contains(Reason.KEYS_OR_RANGES))
+ sb.append(String.format("Txn attempted to access keys or ranges %s that are not longer valid globally (%d -> %s)", keysOrRanges, topology.epoch(), topology.ranges()));
+ if (reason.contains(Reason.HOME_KEY))
+ {
+ if (sb.length() != 0)
+ sb.append('\n');
+ sb.append(String.format("HomeKey %s exists for a range that is no longer globally valid (%d -> %s)", homeKey, topology.epoch(), topology.ranges()));
+ }
+ return sb.toString();
+ }
+
+ @Nullable
+ public static TopologyMismatch checkForMismatch(Topology t, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ EnumSet<TopologyMismatch.Reason> reasons = EnumSet.noneOf(TopologyMismatch.Reason.class);
Review Comment:
Can we set this to null and only allocate the EnumSet if we encounter a problem?
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
I think the SafeCommandStore/SafeCommand would mostly address this coverage? It should throw if you attempt to load something that wasn't included in the PreLoadContext
##########
accord-core/src/main/java/accord/messages/WaitUntilApplied.java:
##########
@@ -170,6 +170,7 @@ private ReadNack apply(SafeCommandStore safeStore, SafeCommand safeCommand)
}
else
{
+ // TODO (debug): this will cause us to send a Nack back, so why are we listening?
Review Comment:
reminder to remove
##########
accord-core/src/main/java/accord/utils/async/AsyncChains.java:
##########
@@ -363,6 +477,11 @@ protected void checkNextIsHead()
Invariants.checkState(next instanceof Head<?>, "Next is not an instance of AsyncChains.Head (it is %s); was map/flatMap called on the same object multiple times?", next.getClass());
}
+ public static AsyncChain<?> detectLeak(Consumer<Throwable> onLeak, Runnable onCall)
Review Comment:
this is cool, WDYT about adding a flag to include this in every chain that can be used in testing?
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309148731
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -871,31 +903,41 @@ public static class Synchronized extends InMemoryCommandStore
{
Runnable active = null;
final Queue<Runnable> queue = new ConcurrentLinkedQueue<>();
+ private final AtomicBoolean running = new AtomicBoolean(false);
public Synchronized(int id, NodeTimeService time, Agent agent, DataStore store, ProgressLog.Factory progressLogFactory, EpochUpdateHolder epochUpdateHolder)
{
super(id, time, agent, store, progressLogFactory, epochUpdateHolder);
}
- private synchronized void maybeRun()
+ private void maybeRun()
{
- if (active != null)
+ if (!running.compareAndSet(false, true))
Review Comment:
this is not related to this patch but was from a different patch I had... I added more messages tests that used this class and I hit a deadlock where one store (S1) was trying to enqueue work to a different store (S2) and that store was trying to enqueue work in the other store (S1)...
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309196117
##########
accord-core/src/test/java/accord/topology/TopologyRandomizer.java:
##########
@@ -26,64 +26,99 @@
import java.util.Map;
import java.util.Set;
import java.util.TreeSet;
+import java.util.concurrent.atomic.AtomicInteger;
import java.util.function.BiFunction;
import java.util.function.Function;
import java.util.function.Supplier;
import javax.annotation.Nullable;
+import com.google.common.annotations.VisibleForTesting;
import com.google.common.collect.Sets;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import accord.burn.TopologyUpdates;
-import accord.impl.IntHashKey;
-import accord.impl.IntHashKey.Hash;
+import accord.impl.PrefixedIntHashKey;
+import accord.impl.PrefixedIntHashKey.Hash;
import accord.local.Node;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.utils.Invariants;
import accord.utils.RandomSource;
+import org.agrona.collections.IntHashSet;
// TODO (required, testing): add change replication factor
public class TopologyRandomizer
{
+ public interface Listener
+ {
+ void onUpdate(Topology topology);
+ }
+
+ public enum Listeners implements Listener
+ {
+ NOOP
+ {
+ @Override
+ public void onUpdate(Topology topology)
+ {
+
+ }
+ }
+ }
+
+ private static class State
Review Comment:
`addPrefix`/`removePrefix` needs to know what is the highest prefix seen so far so have a counter for that, but since we are using enums we can't pass this state into `<init>` and instead have to pass this into the call site... I "could" just pass in the counter to all functions but felt this way was slightly better... more than glad to rework this as I really don't like it
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1446674622
##########
accord-core/src/main/java/accord/coordinate/TxnExecute.java:
##########
@@ -74,8 +75,9 @@ private TxnExecute(Node node, TxnId txnId, Txn txn, FullRoute<?> route, Particip
}
@Override
- protected void start(Set<Id> readSet)
+ protected void start(Iterable<Id> to)
{
+ Set<Id> readSet = Sets.newHashSet(to);
Review Comment:
ill make the change but I wonder if its not best to just do `List.contains`; I went with `Iterable` to make sure I didn't break anything... I feel like in practice the read set will be small as its 1 node per non-overlapping range.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408556525
##########
accord-core/src/main/java/accord/utils/Timestamped.java:
##########
@@ -68,6 +71,6 @@ public static <T> Timestamped<T> mergeEqual(Timestamped<T> a, Timestamped<T> b,
@Override
public String toString()
{
- return data.toString();
+ return toString.apply(data);
Review Comment:
`data` is almost always `int[]` which is useless... so logs on error didn't make sense without 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: 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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1394894159
##########
accord-core/src/main/java/accord/coordinate/TopologyMismatch.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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 accord.coordinate;
+
+import java.util.EnumSet;
+
+import javax.annotation.Nullable;
+
+import accord.api.RoutingKey;
+import accord.primitives.Routables;
+import accord.primitives.Seekables;
+import accord.primitives.TxnId;
+import accord.topology.Topology;
+
+public class TopologyMismatch extends CoordinationFailed
+{
+ public final Topology topology;
+ public final Seekables<?, ?> keysOrRanges;
+ private final EnumSet<Reason> reasons;
+
+ public enum Reason { HOME_KEY, KEYS_OR_RANGES }
+
+ private TopologyMismatch(EnumSet<Reason> reasons, Topology topology, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ super(txnId, homeKey, buildMessage(reasons, topology, homeKey, keysOrRanges));
+ this.reasons = reasons;
+ this.topology = topology;
+ this.keysOrRanges = keysOrRanges;
+ }
+
+ private static String buildMessage(EnumSet<Reason> reason, Topology topology, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ StringBuilder sb = new StringBuilder();
+ if (reason.contains(Reason.KEYS_OR_RANGES))
+ sb.append(String.format("Txn attempted to access keys or ranges %s that are not longer valid globally (%d -> %s)", keysOrRanges, topology.epoch(), topology.ranges()));
+ if (reason.contains(Reason.HOME_KEY))
+ {
+ if (sb.length() != 0)
+ sb.append('\n');
+ sb.append(String.format("HomeKey %s exists for a range that is no longer globally valid (%d -> %s)", homeKey, topology.epoch(), topology.ranges()));
+ }
+ return sb.toString();
+ }
+
+ @Nullable
+ public static TopologyMismatch checkForMismatch(Topology t, TxnId txnId, RoutingKey homeKey, Seekables<?, ?> keysOrRanges)
+ {
+ EnumSet<TopologyMismatch.Reason> reasons = null;
+ if (!t.ranges().contains(homeKey))
+ {
+ if (reasons == null)
+ reasons = EnumSet.noneOf(TopologyMismatch.Reason.class);
+ reasons.add(TopologyMismatch.Reason.HOME_KEY);
+ }
+ Seekables<?, ?> updatedRanges = keysOrRanges.slice(t.ranges(), Routables.Slice.Minimal);
+ if (!updatedRanges.containsAll(keysOrRanges))
Review Comment:
I did ranges.containsAll(keysOrRanges) as that is logically the same.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443158445
##########
accord-core/src/main/java/accord/coordinate/Barrier.java:
##########
@@ -66,7 +66,7 @@ public class Barrier<S extends Seekables<?, ?>> extends AsyncResults.AbstractRes
private final BarrierType barrierType;
@VisibleForTesting
- CoordinateSyncPoint<S> coordinateSyncPoint;
+ AsyncResult<SyncPoint<S>> coordinateSyncPoint;
Review Comment:
yes, as we may return a `accord.utils.async.AsyncResults.Immediate` with a failure.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443380552
##########
accord-core/src/main/java/accord/primitives/AbstractRanges.java:
##########
@@ -56,7 +56,10 @@ public int indexOf(RoutableKey key)
@Override
public int indexOf(Range find)
Review Comment:
pushed. `indexOf` is now in `AbstractKeys` and `AbstractRanges` but they don't share the same signature; ranges asks for a search op. I added a new `firstIndexOf` that uses `CEIL` with the new method in the single place that called this before.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443508483
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
Review Comment:
`BurnTest` now owns the hash function, so able to remove this check. It was here because a key outside of that range wouldn't be known in the `Cluster`, so fail fast. Now that we rely on a truncated hash range for keys this safety check doesn't matter.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443488059
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
Review Comment:
ill get back to this comment later, you have a different comment this depends on so best to address things there.
https://github.com/apache/cassandra-accord/pull/65#discussion_r1442889538
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443252045
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
made 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: 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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443383557
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
if (isRangeQuery)
{
- int rangeCount = 1 + random.nextInt(2);
- List<Range> requestRanges = new ArrayList<>();
- while (--rangeCount >= 0)
- {
- int j = 1 + random.nextInt(0xffff), i = Math.max(0, j - (1 + random.nextInt(0x1ffe)));
- requestRanges.add(IntHashKey.range(forHash(i), forHash(j)));
- }
- Ranges ranges = Ranges.of(requestRanges.toArray(new Range[0]));
- ListRead read = new ListRead(random.decide(readInCommandStore) ? Function.identity() : executor, ranges, ranges);
- ListQuery query = new ListQuery(client, count);
- ListRequest request = new ListRequest(new Txn.InMemory(ranges, read, query, null), listener);
- packets.add(new Packet(client, node, count, request));
+ description = "range";
+ gen = n -> {
+ int[] prefixes = prefixes(n.topology().current());
+
+ int rangeCount = 1 + random.nextInt(2);
+ List<Range> requestRanges = new ArrayList<>();
+ while (--rangeCount >= 0)
+ {
+ int prefix = random.pickInt(prefixes);
+ int i = random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ int j = 1 + random.nextInt(i, HASH_RANGE_END);
Review Comment:
if you prefer uniform I can do a simple `int rangeSize = rs.nextInt(0, 1 << 13) + 1`. If you feel there isn't value in zipf then the complexity of switching between them doesn't seem worth it
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408563786
##########
accord-core/src/test/java/accord/utils/RandomSourceTest.java:
##########
@@ -19,25 +19,22 @@
package accord.utils;
import java.util.Arrays;
-import java.util.Random;
import org.junit.jupiter.api.Assertions;
import org.junit.jupiter.api.Test;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
+import static accord.utils.RandomTestRunner.test;
+
public class RandomSourceTest
{
private static final Logger logger = LoggerFactory.getLogger(RandomSourceTest.class);
@Test
public void testBiasedInts()
{
- RandomSource random = RandomSource.wrap(new Random());
- long seed = random.nextLong();
- logger.info("Seed: {}", seed);
- random.setSeed(seed);
- testBiasedInts(random, 1000, 100000, 0.01, 0.1);
+ test().check(random -> testBiasedInts(random, 1000, 100000, 0.01, 0.1));
Review Comment:
this is also in https://issues.apache.org/jira/browse/CASSANDRA-19114, its a small change so hope that can go in quickly... I just want stable CI so had to fix this flakey test
##########
accord-core/src/test/java/accord/utils/RandomTestRunner.java:
##########
@@ -0,0 +1,69 @@
+/*
+ * 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 accord.utils;
+
+import java.util.function.Consumer;
+
+/**
+ * The main difference between this class and {@link Property} is that this class does not reference {@link Gen} and some
+ * authors wish to avoid that interface, but need to correctly work with randomness in a test that is reproducable when
+ * issues are found from CI.
+ *
+ * {@link Builder#check(Consumer)} is functinally the same as the following from {@link Property}
+ *
+ * {@code qt().withExamples(1).check(testcase); }
+ */
+public class RandomTestRunner
Review Comment:
this is in https://issues.apache.org/jira/browse/CASSANDRA-19114
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1411268765
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
yep that makes sense, thanks
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1411326316
##########
accord-core/src/test/java/accord/Utils.java:
##########
@@ -118,6 +123,16 @@ public static Txn writeTxn(Ranges ranges)
return new Txn.InMemory(ranges, MockStore.read(ranges), MockStore.QUERY, MockStore.update(ranges));
}
+ public static Txn listWriteTxn(Node.Id client, Keys keys)
+ {
+ ListUpdate update = new ListUpdate(Function.identity());
+ for (Key k : keys)
+ update.put(k, 1);
Review Comment:
I don't understand the problem, wouldn't ListUpdate just append another 1 if this was run twice?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442852401
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
This should invoke `Propose.proposeInvalidate` not `Invalidate.invalidate`
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443250027
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
+ int[] keys = new int[keyCount];
+ {
+ IntHashSet seen = new IntHashSet();
+ for (int i = 0; i < keyCount; i++)
+ {
+ int hash;
+ do
+ {
+ // start is exclusive, and end is inclusive; which is the oposite of nextInt, so +1 to account for this
+ hash = 1 + random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ }
+ while (!seen.add(hash));
+ keys[i] = CRCUtils.reverseCRC32LittleEnding(hash);
Review Comment:
> but what value do we derive from reverse lookups
In `BurnTest` no value, but in other tests it makes it so we can be very explicit what ranges we are interacting with by generating keys within those ranges. I use this property to create keys within a given range and outside that range so we can see how we behave under different conditions.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443501284
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
+ int[] keys = new int[keyCount];
+ {
+ IntHashSet seen = new IntHashSet();
+ for (int i = 0; i < keyCount; i++)
+ {
+ int hash;
+ do
+ {
+ // start is exclusive, and end is inclusive; which is the oposite of nextInt, so +1 to account for this
+ hash = 1 + random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ }
+ while (!seen.add(hash));
+ keys[i] = CRCUtils.reverseCRC32LittleEnding(hash);
Review Comment:
> I don't think we need to use the same hashing behaviour between tests though?
Agree
> It would be nicer to support hash collisions IMO
Let me take a look at this again. The reason for this line `keys[i] = CRCUtils.reverseCRC32LittleEnding(hash);` was to try to respect `keyCount` (I interpret as number of unique keys to test again) and the limited hash range you had desired. Since `PrefixedIntHashKey` uses crc32 for the hashing and doesn't mod like the other hash impl, the only way to limit the hash range would be to reverse the hash to get the keys.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443509048
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
+ int[] keys = new int[keyCount];
+ {
+ IntHashSet seen = new IntHashSet();
+ for (int i = 0; i < keyCount; i++)
+ {
+ int hash;
+ do
+ {
+ // start is exclusive, and end is inclusive; which is the oposite of nextInt, so +1 to account for this
+ hash = 1 + random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ }
+ while (!seen.add(hash));
+ keys[i] = CRCUtils.reverseCRC32LittleEnding(hash);
Review Comment:
`BurnTest` now owns the hash function, so supports collisions. I kept the previous keys rather than randomly selecting keys like my previous patch.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442872820
##########
accord-core/src/main/java/accord/primitives/AbstractRanges.java:
##########
@@ -56,7 +56,10 @@ public int indexOf(RoutableKey key)
@Override
public int indexOf(Range find)
Review Comment:
I think we should rename `indexOf` to `ceilIndexOf` just so callers are aware it's not necessarily unique. Or have it require the `Search` parameter.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1394874358
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
honestly I am fine pushing this into its own stand alone patch, not related to the core changes, so if can get the rest in faster, thats bette
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309173411
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -243,6 +283,12 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
if (i == requests.length - 1)
onSubmitted.get().run();
}
+ if (packet.message instanceof Reply.FailureReply)
Review Comment:
hit a NPE in the `List*` code which hit this case... so assuming the `List*` is bug free we should never hit this, but rather than hitting a class cast felt that the original exception was best so adding directly to the failures was better
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309191845
##########
accord-core/src/test/java/accord/topology/TopologyManagerTest.java:
##########
@@ -277,4 +304,280 @@ void truncateTopologyCantTruncateUnsyncedEpochs()
{
}
+
+ @Test
+ void removeRanges()
+ {
+ qt().withExamples(100).check(rs -> {
+ long epochCounter = rs.nextInt(1, 42);
+ boolean withUnchange = rs.nextBoolean();
+ List<Topology> topologies = new ArrayList<>(withUnchange ? 3 : 2);
+ topologies.add(topology(epochCounter++,
+ shard(PrefixedIntHashKey.range(0, 0, 100), idList(1, 2, 3), idSet(1, 2)),
+ shard(PrefixedIntHashKey.range(1, 0, 100), idList(1, 2, 3), idSet(1, 2))));
+ if (withUnchange)
+ topologies.add(topology(epochCounter++,
+ shard(PrefixedIntHashKey.range(0, 0, 100), idList(1, 2, 3), idSet(1, 2)),
+ shard(PrefixedIntHashKey.range(1, 0, 100), idList(1, 2, 3), idSet(1, 2))));
+ topologies.add(topology(epochCounter++,
+ shard(PrefixedIntHashKey.range(1, 0, 100), idList(1, 2, 3), idSet(1, 2))));;
+ History history = new History(new TopologyManager(SUPPLIER, ID), topologies.iterator()) {
+
+ @Override
+ protected void postTopologyUpdate(int id, Topology t)
+ {
+ test(t);
+ }
+
+ @Override
+ protected void postEpochSyncComplete(int id, long epoch, Node.Id node)
+ {
+ test(tm.globalForEpoch(epoch));
+ }
+
+ private void test(Topology topology)
+ {
+ Ranges ranges = topology.ranges();
+ for (int i = 0; i < 10; i++)
+ {
+ Unseekables<?> unseekables = TopologyUtils.select(ranges, rs);
+ long maxEpoch = topology.epoch();
+ long minEpoch = tm.minEpoch() == maxEpoch ? maxEpoch : rs.nextLong(tm.minEpoch(), maxEpoch + 1);
+ assertThat(tm.preciseEpochs(unseekables, minEpoch, maxEpoch))
+ .isNotEmpty()
+ .epochsBetween(minEpoch, maxEpoch)
+ .containsAll(unseekables)
+ .topology(maxEpoch, a -> a.isNotEmpty());
+
+ assertThat(tm.withUnsyncedEpochs(unseekables, minEpoch, maxEpoch))
+ .isNotEmpty()
+ .epochsBetween(minEpoch, maxEpoch, false) // older epochs are allowed
+ .containsAll(unseekables)
+ .topology(maxEpoch, a -> a.isNotEmpty());
+ }
+ }
+ };
+ history.run(rs);
+ });
+ }
+
+ /**
+ * The ABA problem is a problem with registers where you set the value A, then B, then A again; when you observe you see A... which A?
+ */
+ @Test
+ void aba()
+ {
+ TopologyManager service = new TopologyManager(SUPPLIER, ID);
+ List<Node.Id> dc1Nodes = idList(1, 2, 3);
+ Set<Node.Id> dc1Fp = idSet(1, 2);
+ List<Node.Id> dc2Nodes = idList(4, 5, 6);
+ Set<Node.Id> dc2Fp = idSet(4, 5);
+ addAndMarkSynced(service, topology(1,
+ shard(PrefixedIntHashKey.range(0, 0, 100), dc2Nodes, dc2Fp),
+ shard(PrefixedIntHashKey.range(1, 0, 100), dc1Nodes, dc1Fp)));
+ addAndMarkSynced(service, topology(2,
+ shard(PrefixedIntHashKey.range(1, 0, 100), dc1Nodes, dc1Fp)));
+ addAndMarkSynced(service, topology(3,
+ shard(PrefixedIntHashKey.range(0, 0, 100), dc2Nodes, dc2Fp),
+ shard(PrefixedIntHashKey.range(1, 0, 100), dc1Nodes, dc1Fp)));
+
+ // prefix=0 was added in epoch=1, removed in epoch=2, and added back to epoch=3; the ABA problem
+ RoutingKeys unseekables = RoutingKeys.of(PrefixedIntHashKey.forHash(0, 42));
+
+ for (Supplier<Topologies> fn : Arrays.<Supplier<Topologies>>asList(() -> service.preciseEpochs(unseekables, 1, 3),
+ () -> service.withUnsyncedEpochs(unseekables, 1, 3)))
+ {
+ assertThat(fn.get())
+ .isNotEmpty()
+ .epochsBetween(1, 3)
+ .containsAll(unseekables)
+ .topology(1, a -> a.isEmpty())
+ .topology(2, a -> a.isEmpty())
+ .topology(3, a -> a.isNotEmpty()
+ .isRangesEqualTo(PrefixedIntHashKey.range(0, 0, 100))
+ .isHostsEqualTo(dc2Nodes));
+ }
+ }
+
+ @Test
+ void fuzz()
Review Comment:
this test was very useful to make sure the different mutations work with add/remove prefix. I believe I got some of the fixes into earlier commits [1] so you don't see them there... this test is old for me... I found that it was far faster to hit issues here than in `BurnTest`
[1]
```
commit 7c15f3a6203939bc6cb398e538df1ca3557cbe03
Author: Benedict Elliott Smith <be...@apache.org>
Date: Tue Aug 15 16:51:22 2023 +0100
State Eviction (#50)
Permit the state machine to erase transactions that are known to be applied across the cluster.
```
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309176680
##########
accord-core/src/test/java/accord/coordinate/tracking/TrackerReconcilerTest.java:
##########
@@ -67,7 +67,14 @@ void test(int count, BiFunction<RandomSource, Topologies, ? extends TrackerRecon
static <ST extends ShardTracker, T extends AbstractTracker<ST>, E extends Enum<E>>
void test(long seed, BiFunction<RandomSource, Topologies, ? extends TrackerReconciler<ST, T, E>> constructor)
{
- for (TrackerReconciler<?, ?, ?> test : TrackerReconciler.generate(seed, constructor))
- test.test();
+ try
+ {
+ for (TrackerReconciler<?, ?, ?> test : TrackerReconciler.generate(seed, constructor))
+ test.test();
+ }
+ catch (Throwable t)
+ {
+ throw new AssertionError("Failure for seed " + seed, t);
Review Comment:
unrelated to the core work, but was found when doing the topology refactors... the current test relies on logging which means that CI is unlikely to give us the seed we failed with, making theses property tests not reproducible... by throwing with the seed we should be able to repo issues assuming the property tests are deterministic
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443512142
##########
accord-core/src/main/java/accord/messages/Commit.java:
##########
@@ -275,6 +275,22 @@ public static void commitInvalidate(Node node, TxnId txnId, Unseekables<?> infor
// TODO (expected, safety): this kind of check needs to be inserted in all equivalent methods
Invariants.checkState(untilEpoch >= txnId.epoch());
Invariants.checkState(node.topology().hasEpoch(untilEpoch));
+ // Possible that the invalidation is due to the range no longer existing in untilEpoch, which means that
Review Comment:
> How can this happen? We should only invoke with an untilEpoch we attempted to Accept with, and we shouldn't attempt to Accept in an epoch where the keys don't exist
This is the stack trace that hits it
```
commitInvalidate:287, Commit$Invalidate (accord.messages)
commitInvalidate:270, Commit$Invalidate (accord.messages)
lambda$commitInvalidate$12:365, Recover (accord.coordinate)
run:-1, 1373949107 (accord.coordinate.Recover$$Lambda$689)
withEpoch:297, Node (accord.local)
commitInvalidate:365, Recover (accord.coordinate)
lambda$invalidate$10:358, Recover (accord.coordinate)
accept:-1, 73688220 (accord.coordinate.Recover$$Lambda$686)
onSuccess:202, Propose$Invalidate (accord.coordinate)
onSuccess:137, Propose$Invalidate (accord.coordinate)
accept:-1, 353023213 (accord.messages.SafeCallback$$Lambda$555)
lambda$safeCall$1:74, SafeCallback (accord.messages)
```
this epoch is coming from `accord.coordinate.Recover#commitInvalidate`
```
Timestamp invalidateUntil = recoverOks.stream().map(ok -> ok.executeAt).reduce(txnId, Timestamp::max);
```
the peers that are sending the `executeAt` have the following for `Command`
```
Command@1850137076{[1,5,2(KW),7]:AcceptedInvalidateWithDefinition}
```
This looks to be triggered by `accord.messages.Accept.Invalidate#apply`
> I think it is fine to contact untilEpoch even if some keys are missing?
I believe my original logic that Blake approved allowed this, but since you asked for TM to reject these cases, we had to handle here.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1446485229
##########
accord-core/src/main/java/accord/messages/Commit.java:
##########
@@ -275,6 +275,22 @@ public static void commitInvalidate(Node node, TxnId txnId, Unseekables<?> infor
// TODO (expected, safety): this kind of check needs to be inserted in all equivalent methods
Invariants.checkState(untilEpoch >= txnId.epoch());
Invariants.checkState(node.topology().hasEpoch(untilEpoch));
+ // Possible that the invalidation is due to the range no longer existing in untilEpoch, which means that
Review Comment:
I pushed this change and see that `CommandsTest` is stable with it, also tried to document the "why" for next authors who review
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1445965204
##########
accord-core/src/main/java/accord/messages/Commit.java:
##########
@@ -275,6 +275,22 @@ public static void commitInvalidate(Node node, TxnId txnId, Unseekables<?> infor
// TODO (expected, safety): this kind of check needs to be inserted in all equivalent methods
Invariants.checkState(untilEpoch >= txnId.epoch());
Invariants.checkState(node.topology().hasEpoch(untilEpoch));
+ // Possible that the invalidation is due to the range no longer existing in untilEpoch, which means that
Review Comment:
Ok, so the mistake is that we use a later epoch that is voted on (and recorded as such) on preaccept that wasn't proposed. So I guess we really want to replicate the original epoch we used for sending, which means for any Accept we see, we use the highest timestamp. But for PreAccept we only use TxnId. In this case we *should* never use a future epoch, and we should still invalidate/clear any records we might have created.
So, something like
`Timestamp invalidateUntil = recoverOks.stream().map(ok -> ok.saveStatus.hasBeen(Accepted) ? ok.executeAt : ok.txnId).reduce(txnId, Timestamp::max);`
This isn't perfect, as an `AcceptInvalidate` could mask an `Accept` with a higher timestamp, and cause us to not clear some state. But I'm not sure it's worth agonising over, as eventually it all gets cleaned up. Better to keep things simple.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443380467
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -217,21 +254,41 @@ static void burn(RandomSource random, TopologyFactory topologyFactory, List<Id>
.asLongSupplier(forked);
};
- Verifier verifier = createVerifier(keyCount);
- SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, null);
-
+ SimulatedDelayedExecutorService globalExecutor = new SimulatedDelayedExecutorService(queue, new ListAgent(1000L, failures::add, retryBootstrap, (i1, i2) -> {
+ throw new IllegalAccessError("Global executor should enver get a stale event");
+ }));
+ Int2ObjectHashMap<Verifier> validators = new Int2ObjectHashMap<>();
Function<CommandStore, AsyncExecutor> executor = ignore -> globalExecutor;
MessageListener listener = MessageListener.get();
- Packet[] requests = toArray(generate(random, listener, executor, clients, nodes, keyCount, operations), Packet[]::new);
+ if (keyCount > ((long) HASH_RANGE_END - (long) HASH_RANGE_START + 1L))
+ throw new AssertionError(String.format("Attempted to create %d keys which is larger than the range (%d, %d]", keyCount, HASH_RANGE_START, HASH_RANGE_END));
+ int[] keys = new int[keyCount];
+ {
+ IntHashSet seen = new IntHashSet();
+ for (int i = 0; i < keyCount; i++)
+ {
+ int hash;
+ do
+ {
+ // start is exclusive, and end is inclusive; which is the oposite of nextInt, so +1 to account for this
+ hash = 1 + random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ }
+ while (!seen.add(hash));
+ keys[i] = CRCUtils.reverseCRC32LittleEnding(hash);
Review Comment:
I don't think we need to use the same hashing behaviour between tests though? It would be nicer to support hash collisions IMO, as this tests additional functionality. Though we could force collisions if we wanted as well, by reducing the hash range rather than hard coding it.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443280250
##########
accord-core/src/main/java/accord/primitives/AbstractRanges.java:
##########
@@ -56,7 +56,10 @@ public int indexOf(RoutableKey key)
@Override
public int indexOf(Range find)
Review Comment:
I would prefer to remove `indexOf` from `Routables` and introduce a new `indexOf` method only for `RoutableKey` types (e.g. RoutingKeys and Keys). We shouldn't really _have_ an `indexOf` method that accepts a `Range` for `AbstractRanges` as its poorly defined.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442854352
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
I think we should also set `initialPreAcceptIsDone=true` else we might yet complete successfully as well as propose invalidation, which would be a serious fault.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442863467
##########
accord-core/src/main/java/accord/coordinate/ReadCoordinator.java:
##########
@@ -251,7 +251,7 @@ protected void start(Set<Id> to)
public void start()
{
- Set<Id> contact = newHashSetWithExpectedSize(maxShardsPerEpoch());
+ Set<Id> contact = newLinkedHashSetWithExpectedSize(maxShardsPerEpoch());
Review Comment:
I think it is a vestigial requirement that this be a set. We can just allocate a `List`, and modify the `start` method to accept a `List`.
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1323351402
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
We have a lot of places that try to access data outside of the `PreLoadContext` and acts on the global cache.
`accord.impl.InMemoryCommandStore#ifPresent(accord.primitives.TxnId)`
`accord.impl.InMemoryCommandStore.InMemorySafeStore#getIfLoaded(accord.primitives.TxnId)`
`accord.impl.AbstractSafeCommandStore#getInternalIfLoadedAndInitialised`
`accord.local.SafeCommandStore#ifLoadedAndInitialised`
`accord.local.Commands.NotifyWaitingOn#accept`
```
SafeCommand curSafe = ifLoadedAndInitialised(safeStore, depth);
Command cur = curSafe != null ? curSafe.current() : null;
if (cur == null)
{
// need to load; schedule execution for later
safeStore.commandStore().execute(this, this).begin(safeStore.agent());
return;
}
```
Cases like this would be 100% cache hit in BurnTest, so the non-cache hit case was never tested.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408533166
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
I back ported my test from the drop keyspace branch and in a debugger I see `accord.coordinate.Invalidate#Invalidate`
```
Topologies topologies = node.topology().forEpoch(invalidateWith, txnId.epoch());
txnId = {TxnId@3550} "[1,5,2(KW),1]"
invalidateWith = {FullKeyRoute@3548} "{homeKey:1#51196,[1#26558,1#51196,1#54417]}"
this.invalidateWith = {FullKeyRoute@3548} "{homeKey:1#51196,[1#26558,1#51196,1#54417]}"
```
this uses topology `[Topology{epoch=1, [Shard[1#21843,1#29124]:(-1655615791f, -1170363843f, -942051380f), Shard[1#50967,1#58248]:(1f, 1298436801f, 1702745641f)]}]`
so, rather than using the new epoch which doesn't include the range, we use the range from the txn_id.epoch (which is still valid)
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443264669
##########
accord-core/src/main/java/accord/primitives/AbstractRanges.java:
##########
@@ -56,7 +56,10 @@ public int indexOf(RoutableKey key)
@Override
public int indexOf(Range find)
Review Comment:
I am not a fan of this as this method is coming from `accord.primitives.Routables#indexOf` and that doesn't really make sense to me with the other 2 implementations (finding a key in a set of keys).
One possibility is to add a *new* method on `AbstractRanges` that lets you define the `Search` param and have `indexOf` return the *first* match. The only code path I see that might use this new API is `accord.primitives.AbstractRanges#intersects(accord.primitives.Range)` as it could possible do less checks as it can return after the first match
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1442848268
##########
accord-core/src/main/java/accord/coordinate/Barrier.java:
##########
@@ -66,7 +66,7 @@ public class Barrier<S extends Seekables<?, ?>> extends AsyncResults.AbstractRes
private final BarrierType barrierType;
@VisibleForTesting
- CoordinateSyncPoint<S> coordinateSyncPoint;
+ AsyncResult<SyncPoint<S>> coordinateSyncPoint;
Review Comment:
Do we need to make this type less specific?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "belliottsmith (via GitHub)" <gi...@apache.org>.
belliottsmith commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443381296
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
if (isRangeQuery)
{
- int rangeCount = 1 + random.nextInt(2);
- List<Range> requestRanges = new ArrayList<>();
- while (--rangeCount >= 0)
- {
- int j = 1 + random.nextInt(0xffff), i = Math.max(0, j - (1 + random.nextInt(0x1ffe)));
- requestRanges.add(IntHashKey.range(forHash(i), forHash(j)));
- }
- Ranges ranges = Ranges.of(requestRanges.toArray(new Range[0]));
- ListRead read = new ListRead(random.decide(readInCommandStore) ? Function.identity() : executor, ranges, ranges);
- ListQuery query = new ListQuery(client, count);
- ListRequest request = new ListRequest(new Txn.InMemory(ranges, read, query, null), listener);
- packets.add(new Packet(client, node, count, request));
+ description = "range";
+ gen = n -> {
+ int[] prefixes = prefixes(n.topology().current());
+
+ int rangeCount = 1 + random.nextInt(2);
+ List<Range> requestRanges = new ArrayList<>();
+ while (--rangeCount >= 0)
+ {
+ int prefix = random.pickInt(prefixes);
+ int i = random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ int j = 1 + random.nextInt(i, HASH_RANGE_END);
Review Comment:
If we're changing distribution, we should randomise the distribution for each run also, as my experience of zipf is that it actually is often less useful for this kind of test as most values are the same, and they cluster very small. Good distribution behaviour is annoyingly hard. Uniform is not perfect either, but I would ordinarily prefer it over zipf for this kind of input, so we should at least switch between the two.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1443468181
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -70,101 +72,133 @@
import accord.impl.list.ListResult;
import accord.impl.list.ListUpdate;
import accord.local.CommandStore;
+import accord.local.Node;
import accord.local.Node.Id;
import accord.messages.MessageType;
+import accord.messages.Reply;
import accord.primitives.Keys;
import accord.primitives.Range;
import accord.primitives.Ranges;
import accord.primitives.Timestamp;
import accord.primitives.Txn;
+import accord.topology.Shard;
+import accord.topology.Topology;
import accord.utils.DefaultRandom;
import accord.utils.RandomSource;
import accord.utils.async.AsyncExecutor;
+import org.agrona.collections.Int2ObjectHashMap;
+import org.agrona.collections.IntHashSet;
-import static accord.impl.IntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.forHash;
+import static accord.impl.PrefixedIntHashKey.range;
+import static accord.impl.PrefixedIntHashKey.ranges;
import static accord.utils.Utils.toArray;
public class BurnTest
{
private static final Logger logger = LoggerFactory.getLogger(BurnTest.class);
- static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int keyCount, int operations)
- {
- List<Key> keys = new ArrayList<>();
- for (int i = 0 ; i < keyCount ; ++i)
- keys.add(IntHashKey.key(i));
+ private static final int HASH_RANGE_START = 0;
+ private static final int HASH_RANGE_END = 1 << 16;
+ static List<Packet> generate(RandomSource random, MessageListener listener, Function<? super CommandStore, AsyncExecutor> executor, List<Id> clients, List<Id> nodes, int[] keys, int operations)
+ {
List<Packet> packets = new ArrayList<>();
- int[] next = new int[keyCount];
+ Int2ObjectHashMap<int[]> prefixKeyUpdates = new Int2ObjectHashMap<>();
double readInCommandStore = random.nextDouble();
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
if (isRangeQuery)
{
- int rangeCount = 1 + random.nextInt(2);
- List<Range> requestRanges = new ArrayList<>();
- while (--rangeCount >= 0)
- {
- int j = 1 + random.nextInt(0xffff), i = Math.max(0, j - (1 + random.nextInt(0x1ffe)));
- requestRanges.add(IntHashKey.range(forHash(i), forHash(j)));
- }
- Ranges ranges = Ranges.of(requestRanges.toArray(new Range[0]));
- ListRead read = new ListRead(random.decide(readInCommandStore) ? Function.identity() : executor, ranges, ranges);
- ListQuery query = new ListQuery(client, count);
- ListRequest request = new ListRequest(new Txn.InMemory(ranges, read, query, null), listener);
- packets.add(new Packet(client, node, count, request));
+ description = "range";
+ gen = n -> {
+ int[] prefixes = prefixes(n.topology().current());
+
+ int rangeCount = 1 + random.nextInt(2);
+ List<Range> requestRanges = new ArrayList<>();
+ while (--rangeCount >= 0)
+ {
+ int prefix = random.pickInt(prefixes);
+ int i = random.nextInt(HASH_RANGE_START, HASH_RANGE_END);
+ int j = 1 + random.nextInt(i, HASH_RANGE_END);
Review Comment:
got back from lunch and implemented logic that each run does 1 of 4 things
50% chance to select uniform
50% chance to select zipf
When in zipf you have a uniform chance to select the following bias: small, large, random.
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1391257698
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
ill need to look closer at this change, after rebase we are failing and looks like its a cache hit issue... not sure why yet... if I disable cache logic burn test fails and not sure why yet, so can't say this logic is broken or some other logic is actually broken...
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "bdeggleston (via GitHub)" <gi...@apache.org>.
bdeggleston commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1394956498
##########
accord-core/src/main/java/accord/coordinate/CoordinatePreAccept.java:
##########
@@ -250,6 +250,16 @@ void onNewEpoch(Topologies prevTopologies, Timestamp executeAt, List<PreAcceptOk
// TODO (desired, efficiency): check if we have already have a valid quorum for the future epoch
// (noting that nodes may have adopted new ranges, in which case they should be discounted, and quorums may have changed shape)
node.withEpoch(executeAt.epoch(), () -> {
+ TopologyMismatch mismatch = TopologyMismatch.checkForMismatch(node.topology().globalForEpoch(executeAt.epoch()), txnId, route.homeKey(), txn.keys());
+ if (mismatch != null)
+ {
+ Invalidate.invalidate(node, txnId, route, (outcome, failure) -> {
Review Comment:
so presumably we're invalidating this txn because it contains keys that don't intersect with the accord topology, ie: are unreplicated from the perspective of accord. If we try to invalidate with those same keys, won't the invalidation be unable to make progress because it's trying to find replicas for unreplicated keys?
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1408555656
##########
accord-core/src/main/java/accord/utils/RandomSource.java:
##########
@@ -233,6 +238,93 @@ default double nextDouble(double minInclusive, double maxExclusive)
double nextGaussian();
+ default int pickInt(int first, int second, int... rest)
Review Comment:
This was removed in CASSANDRA-19045, adding it back
--
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
Re: [PR] CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces [cassandra-accord]
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell merged PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1323364390
##########
accord-core/src/main/java/accord/utils/async/AsyncChains.java:
##########
@@ -363,6 +477,11 @@ protected void checkNextIsHead()
Invariants.checkState(next instanceof Head<?>, "Next is not an instance of AsyncChains.Head (it is %s); was map/flatMap called on the same object multiple times?", next.getClass());
}
+ public static AsyncChain<?> detectLeak(Consumer<Throwable> onLeak, Runnable onCall)
Review Comment:
> WDYT about adding a flag to include this in every chain that can be used in testing?
I currently use `accord.utils.Invariants#isParanoid` in `accord.impl.basic.DelayedCommandStores.DelayedCommandStore#submit(java.util.concurrent.Callable<T>)`, so `CommandStore` leaks should be detected. Are there other places you are thinking about? We have `accord.utils.async.AsyncChains.Head` but that feels tricky as we use `extends`, so would need a costly `finalize` or a big refactor to move away from `Head` directly.
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1324856324
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
s/a lot/a few/g
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309186123
##########
accord-core/src/test/java/accord/topology/TopologyManagerTest.java:
##########
@@ -277,4 +304,280 @@ void truncateTopologyCantTruncateUnsyncedEpochs()
{
}
+
+ @Test
+ void removeRanges()
Review Comment:
BurnTest does not include remove range support, but TM does in this patch, so this test is still relevant. The remove range in BurnTest has issues with multiple subsystems so was punted, but this subsystem was low effort so kept
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309114286
##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -92,6 +94,11 @@ public abstract class InMemoryCommandStore extends CommandStore
private final TreeMap<TxnId, RangeCommand> rangeCommands = new TreeMap<>();
private final TreeMap<TxnId, Ranges> historicalRangeCommands = new TreeMap<>();
+ /**
+ * Since this cache is fully in-memory the store does not hit states that most stores will; that data is not in-memory!
+ * To simulate such behaviors, a "cache" is used to state what is in-memory vs what needs to be loaded.
+ */
+ private final Set<Object> cache = new LinkedHashSet<>();
Review Comment:
this was unrelated to add/remove keyspace patch, it was mostly due to an observation we had a lot of code checking if things are cached to avoid loading and since this class is 100% in-memory we were at 100% cache hit rate, so went with faking a cache miss to get better coverage.
--
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
[GitHub] [cassandra-accord] dcapwell commented on a diff in pull request #65: CASSANDRA-18804: (Accord): Bug fixes from CASSANDRA-18675 to better support adding keyspaces
Posted by "dcapwell (via GitHub)" <gi...@apache.org>.
dcapwell commented on code in PR #65:
URL: https://github.com/apache/cassandra-accord/pull/65#discussion_r1309172166
##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -92,72 +102,101 @@ static List<Packet> generate(RandomSource random, Function<? super CommandStore,
for (int count = 0 ; count < operations ; ++count)
{
+ int finalCount = count;
Id client = clients.get(random.nextInt(clients.size()));
Id node = nodes.get(random.nextInt(nodes.size()));
boolean isRangeQuery = random.nextBoolean();
+ String description;
+ Function<Node, Txn> gen;
Review Comment:
since we don't know what topologies exist until we run on a node, moved the current logic into a lambda that runs in the node so we know what prefixes could be accessed...
--
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