CASSANDRA-18519: (CEP-15: (C*) Add notion of CommandsForRanges and make this durable in C*) by dcapwell · Pull Request #46 · apache/cassandra-accord

dcapwell · 2023-05-16T20:33:56Z

No description provided.

dcapwell · 2023-05-16T20:38:46Z

accord-core/src/main/java/accord/impl/CommandTimeseries.java

+import static accord.utils.Utils.ensureSortedImmutable;
+import static accord.utils.Utils.ensureSortedMutable;
+
+public class CommandTimeseries<D>


just moved this out of CommandsForKeys so that the range logic didn't have to duplicate anything... I just build this time series from the data on-demand

dcapwell · 2023-05-16T20:39:08Z

accord-core/src/main/java/accord/impl/CommandTimeseries.java

+    public enum TestTimestamp
+    {BEFORE, AFTER}
+
+    private final Seekable keyOrRange;


this type change is the only real change to this class

dcapwell · 2023-05-16T20:42:14Z

accord-core/src/main/java/accord/impl/CommandTimeseriesHolder.java

+
+package accord.impl;
+
+public interface CommandTimeseriesHolder


not a fan of the name, so any recommendations welcome =)

… Observable

…. This is to make way for range txn support

dcapwell · 2023-05-18T21:52:42Z

accord-core/src/main/java/accord/topology/Topology.java

    }

-    private <P1> int[] subsetFor(Unseekables<?, ?> select, IndexedPredicate<P1> predicate, P1 param)
+    private enum OnUnknown { REJECT, IGNORE }


this was actually a hack to make progress...

What I saw was that the local epoch=1, but we saw epoch=2, we try to see if there is overlap and fail as epoch=1 doesn't have the new table range that is defined in epoch=2

I changed C* code to defer exec of messages and PreLoadContext until the epoch they depend on is ready... which may remove the need for this hack...

This likely hasn't been found in burn due to working with a single range, so only would impact new ranges being added and used right away (which happens in C* tests)

It kind of makes sense as a work around for the case you are describing, where is this code that tries to compare different epochs that fails?

It seems like this change is opting everything into the IGNORE behavior?

Went with ignore for all cases as I couldn’t find any caller that allowed the failure. At the dr office so can’t show, it changing default and running AccordCQLTest you hit this right away

here is the exception

java.lang.IllegalArgumentException: Range not found for TokenKey{keyspace=demo_ks, key=-9079300900759362424} at accord.topology.Topology.subsetFor(Topology.java:266) at accord.topology.Topology.visitNodeForKeysOnceOrMore(Topology.java:337) at accord.topology.TopologyManager.withUnsyncedEpochs(TopologyManager.java:341) at accord.topology.TopologyManager.withUnsyncedEpochs(TopologyManager.java:310) at accord.coordinate.CoordinatePreAccept.<init>(CoordinatePreAccept.java:129) at accord.coordinate.CoordinateTransaction.<init>(CoordinateTransaction.java:49) at accord.coordinate.CoordinateTransaction.coordinate(CoordinateTransaction.java:54) at accord.local.Node.initiateCoordination(Node.java:447) at accord.local.Node.lambda$coordinate$9(Node.java:439) at accord.local.Node.withEpoch(Node.java:220) at accord.local.Node.coordinate(Node.java:439)

So, the use case is the following

create a new Keyspace w/ table

run a txn over that new table

The org.apache.cassandra.distributed.test.accord.NewSchemaTest test stresses this edge case to make sure the different epochs are supported

Huh, interesting consequence of having the table id in the MSB of the key.

in this case, TokenKey is scoped to the keyspace, so a new table in an existing keyspace is fine, but adding a new keyspace and touching it causes this.

Ah right, I was concerned about this because it causes false conflicts across tables in a key space. Discussed it with Benedict and we might want to scope it differently at some point.

dcapwell · 2023-05-19T00:32:35Z

accord-core/src/main/java/accord/coordinate/RangeUnavailable.java

+import accord.primitives.Ranges;
+import accord.primitives.TxnId;
+
+public class RangeUnavailable extends Exhausted


Exhausted could be different things, so this makes it easier to see what happened; what table ranges could not be read?

accord-core/src/main/java/accord/impl/CommandTimeseries.java

dcapwell · 2023-05-19T16:54:50Z

accord-core/src/main/java/accord/impl/CommandTimeseries.java

+
+        for (D data : (testTimestamp == TestTimestamp.BEFORE ? commands.headMap(timestamp, false) : commands.tailMap(timestamp, false)).values())
+        {
+            TxnId txnId = loader.txnId(data);


I changed the ordering of this method due to cost to deserialize for CommandsForKeys, idea was to only deserialize what is needed and avoid touching deps until needed

…xisting keyspace

aweisberg

Nothing major, some questions. The Topology change is the main thing I need to understand better since it is always opting into IGNORE at the moment.

accord-core/src/main/java/accord/local/CommandStores.java

accord-core/src/main/java/accord/primitives/Ranges.java

accord-core/src/main/java/accord/primitives/TxnId.java

accord-core/src/main/java/accord/topology/Topology.java

aweisberg · 2023-05-22T19:01:17Z

accord-core/src/main/java/accord/topology/Topology.java

    }

-    private <P1> int[] subsetFor(Unseekables<?, ?> select, IndexedPredicate<P1> predicate, P1 param)
+    private enum OnUnknown { REJECT, IGNORE }


It kind of makes sense as a work around for the case you are describing, where is this code that tries to compare different epochs that fails?

It seems like this change is opting everything into the IGNORE behavior?

accord-core/src/main/java/accord/topology/TopologyManager.java

dcapwell · 2023-05-23T19:06:25Z

accord-core/src/main/java/accord/primitives/Ranges.java

        return construct(cachedRanges.completeAndDiscard(result, count));
    }

+    public Map<Boolean, Ranges> partitioningBy(Predicate<? super Range> test)


moved away from the Partition class as this actually is better the more I looked at it... Partition class didn't properly handle this.isEmpty(), but with a map we have a few states

map is empty

map(true)

map(false)

map(true, false)

With this change, we at least have the same signature as Collectors.partitioningBy

dcapwell · 2023-05-23T19:07:01Z

accord-maelstrom/src/test/java/accord/maelstrom/Runner.java

-            Runner.run(nodeCount, new StandardQueue.Factory(randomSource), randomSource::fork, factory, commands);
+            try
+            {
+                Runner.run(nodeCount, new StandardQueue.Factory(randomSource), randomSource::fork, factory, commands);


these tests are not deterministic, so same seed doesn't actually get the same results =(

…an store; needed for C*

dcapwell · 2023-05-24T20:27:56Z

accord-core/src/main/java/accord/impl/InMemoryCommandStore.java

    }

-    @Override
-    protected void registerHistoricalTransactions(Deps deps)


moved this to safe command store... we can't handle this in CommandStore for C* unless we lazy load the dependencies (which requires #27)

…e what to do

aweisberg

+1 TY

belliottsmith · 2023-05-25T21:01:42Z

accord-core/src/main/java/accord/local/SafeCommandStore.java

    NodeTimeService time();
    CommandStores.RangesForEpoch ranges();
    Timestamp maxConflict(Seekables<?, ?> keys, Ranges slice);
+    void registerHistoricalTransactions(Deps deps);


I had intended to keep this to CommandStore to isolate the APIs - SafeCommandStore is for working with Command and SafeCommand whereas CommandStore is for internal book-keeping operations.

sorry, just saw this after merge...

In order to make the outcome of this method durable, it has to be in the safe store for C*, so felt it was best to move it there as that's were we mutate state normally

belliottsmith · 2023-05-25T21:02:23Z

accord-core/src/main/java/accord/local/SaveStatus.java

+            case Invalidated:
+                return true;
+            default:
+                return false;


I would prefer not to use default properties, as it makes it harder to safely modify the state machine in future.

Is this actually used?

it was, but then deleted in favor of something else; but forgot to purge this

dcapwell commented May 16, 2023

View reviewed changes

dcapwell added 16 commits May 16, 2023 14:53

Lazy deserialize fields from CommandsForKey to avoid when not needed

8c99d1c

checkpoint

2a34e82

docs

aa3611b

created new forCallback to help bridge the gap between AsyncChain and…

bed28e1

… Observable

onNext now is allowed to throw checked exception

856127f

refactor to pull CommandTimeseries out and make it work with Seekable…

722ce53

…. This is to make way for range txn support

refactors

c7b03c4

onError and onComplete now required, and added new asChain method

c054e1d

Observable.asChain now takes a collector so we can work with list/set

cfabfda

revert import ordering

4d029dc

fix import after rebase

223beeb

fixed import order

9429814

fix imports

e1835da

added a distinct filter

745c281

improve error msg

16be4e9

trigger ci

37b2e2b

dcapwell force-pushed the CASSANDRA-18519 branch from d5b1d81 to 37b2e2b Compare May 17, 2023 22:21

dcapwell added 5 commits May 18, 2023 10:09

fixed edge case where pending is defined but first epoch not seen

5e958b2

improved error msg

692d55d

added quick hack to allow ignoring unknown ranges

b56f424

added a new ForwardingRoutableKey for interval tree work

0d70e37

remove forwarding check as it doesn't work when you reverse the objects

8a5a56d

dcapwell commented May 18, 2023

View reviewed changes

dcapwell added 3 commits May 18, 2023 15:01

refactor so observable to callback is used when converting to a chain

fb87624

improve the error msg when a range cant be read do to bootstrap

cd3c65c

added way for range search to ignore test kind, as all txn conflict

60740c3

dcapwell commented May 19, 2023

View reviewed changes

accord-core/src/main/java/accord/impl/CommandTimeseries.java Outdated Show resolved Hide resolved

dcapwell commented May 19, 2023

View reviewed changes

dcapwell added 5 commits May 19, 2023 14:07

todo to remove the type override

e9132d7

added logic to allow system to exclude bootstrap based off the change

a2175b5

handle the case where Ranges has a new keyspace and a new range for e…

03fbc29

…xisting keyspace

remove ignoreTestKind as we only want range txn to depend on range tx

8d87f6e

trying to improve maelstorm rand tests

7cdf2f5

aweisberg self-requested a review May 22, 2023 16:36

aweisberg requested changes May 22, 2023

View reviewed changes

dcapwell added 3 commits May 23, 2023 11:01

revert fromTimestamp

56da122

added max to CommandTimeseriesHolder so maxConflict can reuse mapReduce

583a800

switched to Map<Boolean, Ranges> rather than Partition

b86652d

dcapwell commented May 23, 2023

View reviewed changes

dcapwell added 4 commits May 24, 2023 09:55

fixed issue where preload context didnt include keys

a7e9cd9

added TODO

7c82fff

todo

0df6349

refactor so registerHistoricalTransactions is in safe store rather th…

56216a9

…an store; needed for C*

dcapwell commented May 24, 2023

View reviewed changes

dcapwell added 2 commits May 24, 2023 16:41

added a isComplete to status

0e7e8f4

push the reject/ignore logic to the caller so the callers can validat…

9ca49ce

…e what to do

aweisberg approved these changes May 25, 2023

View reviewed changes

belliottsmith reviewed May 25, 2023

View reviewed changes

dcapwell closed this May 25, 2023

dcapwell deleted the CASSANDRA-18519 branch May 25, 2023 23:04


		package accord.impl;

		public interface CommandTimeseriesHolder

Conversation

dcapwell commented May 16, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aweisberg left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants