History Builder API 2.0 #24
Conversation
| // We derive from entropy source here to avoid letting the step change state for other states | ||
| // For example, if you start drawing more entropy bits from one of the steps, but won't change | ||
| // other steps, their states won't change either. |
There was a problem hiding this comment.
I like this description + API, makes the PCG "parallel streams" concept easier to understand for people that are less familiar
| public interface EntropySource | ||
| { | ||
| long next(); | ||
| void seed(long seed); |
There was a problem hiding this comment.
Is re-seeding supported after next is called?
There was a problem hiding this comment.
yeah, you can re-seed any time, at least as of now...
There was a problem hiding this comment.
In what situations would it make sense to re-seed? Seems like this could lead to runs that can't reproduce (unless we record every seed + when it was applied).
| if (pattern.matcher(statement).find()) | ||
| return primary.execute(statement, cl, bindings); |
There was a problem hiding this comment.
Eventually could see an Operation<SUT> API in the future, to make this easier to express
There was a problem hiding this comment.
great idea! if you like, you can open a jira and we hopefully will get to it.
|
|
||
| public CompletableFuture<Object[][]> executeAsync(String statement, ConsistencyLevel cl, Object... bindings) | ||
| { | ||
| return null; |
There was a problem hiding this comment.
Would prefer an UnsupportedOperationException here
| if (schema.trackLts) | ||
| { | ||
| visited_lts_list = (List<Long>) result[result.length - 1]; | ||
| visited_lts_list.sort(Long::compare); |
There was a problem hiding this comment.
Don't we want these in the original order returned by the query?
There was a problem hiding this comment.
Good point. But since we mostly use these to compare with actually visited LTS (which are in monotonically increasing order), we are sorting them here. I could not find a use-case for knowing the order things were inserted in. Maybe we can have a more sophisticated checker that can check query bounds / visibility or something.
| } | ||
|
|
||
| @Override | ||
| public harry.util.BitSet columns() |
There was a problem hiding this comment.
Unnecessary qualified class name
| public CompiledStatement insert(long lts, long pd, long cd, long opId) | ||
| public CompiledStatement insert(VisitExecutor.WriteOp op) |
| CompiledStatement statement = operationInternal(lts, pd, cd, opId, opType); | ||
| statements.add(statement.cql()); | ||
| Collections.addAll(bindings, statement.bindings()); | ||
| hadVisibleVisit = operation.opKind() != OpSelectors.OperationKind.DELETE_PARTITION; |
There was a problem hiding this comment.
Aren't delete range / slice also not visible?
There was a problem hiding this comment.
They are visible since if there is a partition liveness marker, partition's static column is going to survive. Adding a comment!
| // TODO: we can implement a pluggable time source via a custom clock, too; the only limitation is that order | ||
| // of timestamps has to be consistent with LTS order |
There was a problem hiding this comment.
Don't all Harry clocks have this requirement? OffsetClock and ApproximateMonotonicClock both provide monotonicity of RTS given monotonic LTSs.
There was a problem hiding this comment.
Yes, sure. The idea here is that you can come with your own timestamps, not have Harry generate them. Maybe for some weird edge condition testing or something.
| * | ||
| * |
There was a problem hiding this comment.
It seems like this is intended to be a common entrypoint for people to create new Harry tests. Those people are likely to look at this file first. Can you expand on this doc?
There was a problem hiding this comment.
Added quite a bit of doc now
| import harry.visitors.MutatingRowVisitor; | ||
| import harry.visitors.ReplayingVisitor; | ||
|
|
||
| public class HistoryBuilderIntegrationTest extends ModelTestBase |
There was a problem hiding this comment.
nit: Did we mean to delete this file?
There was a problem hiding this comment.
Yes, this is for the old, bad history builder!
|
|
||
| @Override | ||
| public void beginModification(long lts) | ||
| { |
There was a problem hiding this comment.
Unrelated to the patch I guess, but should readingFrom and writingTo be final above?
There was a problem hiding this comment.
yes, final not static! 👍
ifesdjeen
force-pushed
the
CASSANDRA-19116
branch
from
88982f1 to
37adaaf
Compare
| Arrays.copyOf(vds, vds.length), Arrays.copyOf(lts, lts.length)); | ||
| } | ||
|
|
||
| public boolean equals(Object o) |
There was a problem hiding this comment.
nit: @Override ?
I don't know if you'll need a hashCode() here or ever have this in a collection, but I'm compelled by habit to mention it :p
| List<Long> visited_lts) | ||
| { | ||
| assert slts.length == sds.length; | ||
| assert lts.length == vds.length; |
There was a problem hiding this comment.
nit: Brief message be helpful for when these trigger?
| return slts.length > 0; | ||
| } | ||
|
|
||
| public ResultSetRow clone() |
| { | ||
| return new ResultSetRow(pd, cd, | ||
| Arrays.copyOf(sds, sds.length), Arrays.copyOf(slts, slts.length), | ||
| Arrays.copyOf(vds, vds.length), Arrays.copyOf(lts, lts.length)); |
There was a problem hiding this comment.
Does visited_lts need to be part of the clone?
There was a problem hiding this comment.
yes! thank you for spotting!
| Objects.equals(visited_lts, that.visited_lts); | ||
| } | ||
|
|
||
| public String toString() |
There was a problem hiding this comment.
nit: Does visited_lts need to be part of the toString(), or is it something that might grow to be potentially huge and not visually friendly? :D
There was a problem hiding this comment.
yeah, here i would probably leave it out since we display it separately
| return cqlName; | ||
| } | ||
|
|
||
| public boolean equals(Object o) |
There was a problem hiding this comment.
nit: @Override on this and hashCode()
| { | ||
| StringBuilder sb = new StringBuilder(); | ||
| sb.append("UPDATE ").append(schema.keyspace).append(".").append(schema.table) | ||
| .append(" SET visited_lts = visited_lts + [").append(lts).append("] ") |
There was a problem hiding this comment.
So we're guaranteed the schema has visited_lts here?
There was a problem hiding this comment.
added assert in constructor.
| } | ||
|
|
||
| private Reconciler.RowState updateRowState(Reconciler.RowState currentState, List<ColumnSpec<?>> columns, long cd, long[] vds, long lts, boolean writePrimaryKeyLiveness) | ||
| private Reconciler.RowState updateRowState(Reconciler.RowState currentState, List<ColumnSpec<?>> columns, long cd, long[] sds, long lts, boolean writePrimaryKeyLiveness) |
There was a problem hiding this comment.
Stumbling over naming here a bit. We have sds, and methods calling this w/ the argument staticVds or vds. updateRowState() isn't always called w/ static values (or value descriptors) right? (Aside: Would vds for normal column values and svds make sense for static values?)
There was a problem hiding this comment.
Renamed staticVds to sds, and renamed sds here to just vds (i.e. value descriptors).
|
|
||
| switch (opType) | ||
| long lts = operation.lts(); | ||
| assert pdSelector.pd(operation.lts(), schema) == operation.pd(); |
There was a problem hiding this comment.
nit: Quick message for the assert indicating what went wrong?
| assert pdSelector.pd(operation.lts(), schema) == operation.pd(); | ||
|
|
||
| if (operation.opKind() != OpSelectors.OperationKind.DELETE_PARTITION) | ||
| partitionState.visitedLts.add(operation.lts()); |
There was a problem hiding this comment.
Deleting the whole partition doesn't count as a visit?
There was a problem hiding this comment.
extracted into a method and documented:
/**
* All operations apart from partiton delition, including partition hist are visible since if there is a
* partition liveness marker, partition's static column is going to survive. We use this method to match
* computed LTS with tracked ones.
*/
public boolean hasVisibleVisit()
{
return this != OpSelectors.OperationKind.DELETE_PARTITION;
}
| writeOp.vds(), | ||
| lts, | ||
| op.opType == OpSelectors.OperationKind.INSERT || op.opType == OpSelectors.OperationKind.INSERT_WITH_STATICS); | ||
| op.opKind() == OpSelectors.OperationKind.INSERT || op.opKind() == OpSelectors.OperationKind.INSERT_WITH_STATICS); |
There was a problem hiding this comment.
nit: Might work to rename this opKind() -> kind(), so it reads op.kind()
ifesdjeen
force-pushed
the
CASSANDRA-19116
branch
from
ce59efc to
32fe4fa
Compare
| * * Ability to generate multiple "next" random seeds | ||
| * * Ability to generate multiple "dependent" seeds, from which we can retrace the base seed with subtraction | ||
| */ | ||
| public interface RandomGenerator |
There was a problem hiding this comment.
nit: meant to remove the file as well?
There was a problem hiding this comment.
ok i know the reason: patch -p1 does not remove files! Fixed now.
| public boolean nextBoolean() | ||
| { | ||
| return RngUtils.asBoolean(next()); | ||
| } |
There was a problem hiding this comment.
nit: All these nextX() methods and derive() could use an @Override
| primary.shutdown(); | ||
| } | ||
|
|
||
| private static final Pattern pattern = Pattern.compile("select", Pattern.CASE_INSENSITIVE); |
There was a problem hiding this comment.
nit: Should we use ^select just out of paranoia? (I guess you can also just guarantee there won't be random column names like fooSelect :D
| * be taken to map a real-time timestamp from the value retrieved from the database in order | ||
| * to map it back to the logical timestamp of the operation that wrote this value. | ||
| */ | ||
| // TOD: rename to just "Clock", it is not monotonic anymore |
There was a problem hiding this comment.
nit: Something we want to do here or in another patch?
There was a problem hiding this comment.
May as well do it now! Done
| private final int STEPS_PER_ITERATION = 1_000; | ||
| private final int MAX_PARTITIONS = 100; | ||
|
|
||
| protected Configuration.ModelConfiguration modelConfiguration() |
|
|
||
| public interface WriteOp extends Operation | ||
| { | ||
| long pd(); |
There was a problem hiding this comment.
nit: Already inherits pd() from Operation?
| ColumnSpec.regularColumn("v2", ColumnSpec.int64Type)), | ||
| Arrays.asList()) | ||
| .trackLts(); | ||
| ; |
| import harry.visitors.ReplayingVisitor; | ||
| import harry.visitors.VisitExecutor; | ||
|
|
||
| public class HistoryBuilderTest |
There was a problem hiding this comment.
Oh weird, not sure why it has not gotten removed
| } | ||
| } | ||
|
|
||
| public static interface Step<STATE> |
There was a problem hiding this comment.
nit: None of the interfaces need the static keyword
|
|
||
| return true; | ||
| }) | ||
| .run(STEPS_PER_ITERATION, seed); |
There was a problem hiding this comment.
So I'll summarize what I think is going on here, and then my review should be complete...
We create a ModelChecker with single-operation steps to...
1.) Record a series of operations against the model using a HistoryBuilder as the state container.
2.) Replay them all against the SUT.
3.) Validate all the partitions we visited.
There are some details I'm not clear on, though. It looks like ModelChecker#run() executes individual steps, and each step execution adds data to the HistoryBuilder log. Then when we check the exit condition, we actually replay the operations to the SUT from the beginning up to the last visited LTS and validate the model.
Am I one the right track?
There was a problem hiding this comment.
Right, we create a model checker that triggers series of random steps, then replays them against the SUT, and validates visited partitions.
Then when we check the exit condition, we actually replay the operations to the SUT from the beginning up to the last visited LTS and validate the model.
So there are two different history builders: one replaying (with auto-replay on) and one that simply creates series of events, in case you would like to preturb / execute them in some sophisticated way. There was no specific way for us to collect and then replay things, but I have first created a regular history builder, and then replaying one, built on top of the first one.
Patch for CASSANDRA-19116 by Alex Petrov, reviewed by Abe Ratnofsky
ifesdjeen
force-pushed
the
CASSANDRA-19116
branch
from
32fe4fa to
f8d3983
Compare
3 participants
No description provided.