belliottsmith commented on code in PR #26:
URL: https://github.com/apache/cassandra-accord/pull/26#discussion_r1093742649
##########
accord-core/src/main/java/accord/coordinate/Recover.java:
##########
@@ -212,56 +213,63 @@ private void recover()
{
default: throw new IllegalStateException();
case Invalidated:
+ {
commitInvalidate();
return;
+ }
case Applied:
case PreApplied:
- // TODO (desired, efficiency): in some cases we can use
the deps we already have (e.g. if we have a quorum of Committed responses)
- node.withEpoch(executeAt.epoch(), () -> {
- CollectDeps.withDeps(node, txnId, route, txn,
acceptOrCommit.executeAt, (deps, fail) -> {
- if (fail != null)
- {
- accept(null, fail);
- }
- else
- {
- // TODO (required, consider): when
writes/result are partially replicated, need to confirm we have quorum of these
- Persist.persistAndCommit(node, txnId, route,
txn, executeAt, deps, acceptOrCommit.writes, acceptOrCommit.result);
- accept(acceptOrCommit.result, null);
- }
- });
- });
+ {
+ // must have gone through Accepted, so we must have
witnessed >= Accepted for each shard
+ Deps acceptedDeps = tryMergeAcceptedDeps();
+ Invariants.checkState(acceptedDeps != null);
+ // TODO (required, consider): when writes/result are
partially replicated, need to confirm we have quorum of these
+ Persist.persistAndCommit(node, txnId, route, txn,
executeAt, acceptedDeps, acceptOrCommit.writes, acceptOrCommit.result);
+ accept(acceptOrCommit.result, null);
return;
+ }
case ReadyToExecute:
case PreCommitted:
case Committed:
+ {
+ Deps acceptedDeps = tryMergeAcceptedDeps(); // must have
gone through Accepted, so we must have witnessed >= Accepted for each shard
+ Invariants.checkState(acceptedDeps != null);
// TODO (desired, efficiency): in some cases we can use
the deps we already have (e.g. if we have a quorum of Committed responses)
- node.withEpoch(executeAt.epoch(), () -> {
- CollectDeps.withDeps(node, txnId, route, txn,
executeAt, (deps, fail) -> {
- if (fail != null) accept(null, fail);
- else Execute.execute(node, txnId, txn, route,
acceptOrCommit.executeAt, deps, this);
- });
- });
+ Execute.execute(node, txnId, txn, route,
acceptOrCommit.executeAt, acceptedDeps, this);
return;
+ }
case Accepted:
- // no need to preaccept the later round, as future
operations always include every old epoch (until it is fully migrated)
- propose(acceptOrCommit.executeAt, mergeDeps());
- return;
+ {
+ Deps deps;
+ if (txnId.rw().proposesDeps()) deps =
tryMergeAcceptedDeps();
Review Comment:
So, this is something that is probably going to evolve. In practice we
*don't* need to treat them differently today, because we do actually send deps
with accept today, *and* because we are able to invalidate any transaction that
hadn't reached a quorum of accepts (though we don't currently, we probably
*should*, as it is consistent with our behaviour elsewhere of invalidating
transactions we know were not executed by their coordinator).
However (and this is documented, but evidently not well), standard
transactions are happy to get deps that are superset of those they need, and
then filter them. However, exclusive sync points want to _precisely_ exclude
txnId they did not witness. We don't want extra txnId sneaking in next time
around, as that wouldn't have been witnessed by the replicas that wanted to
know for sure they had a complete set of txnId lower than that sync point. So
for these transactions we _require_ that we re-propose precisely the same deps
as the original coordinator _or_ we invalidate the transaction if they cannot
be found.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
