-
Notifications
You must be signed in to change notification settings - Fork 23
Cassandra 19503 #84
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: trunk
Are you sure you want to change the base?
Cassandra 19503 #84
Conversation
…ge txn and instead uses the sync point empty txn for reads
…so rewrote those to use a new createIllegalState method
| void contact(Set<Node.Id> nodes, Topologies topologies, Callback<GetMaxConflictOk> callback) | ||
| { | ||
| node.send(nodes, to -> new GetMaxConflict(to, topologies, route, keysOrRanges, executionEpoch)); | ||
| node.send(nodes, to -> new GetMaxConflict(to, topologies, route, keysOrRanges, executionEpoch), callback); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the fact this happened implies that burn test doesn't hit this code path, so should be something we think about adding... C* hits this all the time as we don't have a max applied, so we would block forever
| // TODO (expected): implement abort | ||
| } | ||
|
|
||
| public static class FetchRequest extends WaitUntilAppliedAndReadData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there was a TODO to merge these 2 so I went ahead and did that
| public static class FetchRequest extends ReadData | ||
| { | ||
| private static final ExecuteOn EXECUTE_ON = new ExecuteOn(Applied, Applied); | ||
| public final PartialTxn read; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
honestly I kept this PartialTxn because it didn't impact anything and I needed to get green before really thinking about it... We only actually care about accord.api.Read so would be a trivial refactor to switch to that
|
|
||
| TopologyMismatch tm = TopologyMismatch.checkForMismatch(snapshot.get(maxEpoch).global(), select); | ||
| EpochState maxState = snapshot.get(maxEpoch); | ||
| Invariants.checkState(maxState != null, "Unable to find epoch %d; known epochs are %d -> %d", maxEpoch, snapshot.minEpoch(), snapshot.currentEpoch); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
in C* we bootstrap with epoch=9 but there are pending txn in epoch=8... this means that we are failing to handle those txn and SimpleProgressLog starts spamming us... I tried to look at how to add historic epochs but it was getting a bit annoying and didn't actually impact the bootstrap tests... so figured I could delay that problem to another patch
| { | ||
| default: throw new AssertionError(); | ||
| case Key: | ||
| if (!keys.contains((Key)key)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the main reason bootstrap regression failed to be detected in burn test was that it switched to using the sync point's emptyTxn, which has 0 keys... since ListRead supports ranges it was able to return the data requested from bootstrap even though its inputs were outside of the range of keys... This check helps protect that from happening again.
6a9482b to
4a8566a
Compare
c3a1d8f to
efc6a38
Compare
289d5ea to
064735b
Compare
2edc7fb to
a271897
Compare
50d6ef9 to
bd0761c
Compare
183d5d0 to
cd7f495
Compare
No description provided.