ifesdjeen commented on code in PR #103:
URL: https://github.com/apache/cassandra-accord/pull/103#discussion_r1679391698
##########
accord-core/src/main/java/accord/local/Bootstrap.java:
##########
@@ -140,6 +140,8 @@ void start(SafeCommandStore safeStore0)
.flatMap(syncPoint -> node.withEpoch(epoch, () ->
store.submit(contextFor(localSyncId, syncPoint.waitFor.keyDeps.keys(),
KeyHistory.COMMANDS), safeStore1 -> {
if (valid.isEmpty()) // we've lost ownership of the range
return AsyncResults.success(Ranges.EMPTY);
+ if (!syncPoint.waitFor.isEmpty() &&
!node.topology().hasEpoch(syncPoint.waitFor.minTxnId().epoch()))
+ throw Invariants.illegalState("Deps has a txn %s that
contains an unknown epoch; min=%d, max=%d", syncPoint.waitFor.minTxnId(),
node.topology().minEpoch(), node.topology().current().epoch());
Review Comment:
Should we throw here or should we return `AyncResults.failure`?
##########
accord-core/src/main/java/accord/topology/TopologyManager.java:
##########
@@ -98,36 +97,33 @@ static class EpochState
this.addedRanges =
global.ranges.subtract(prevRanges).mergeTouching();
this.removedRanges =
prevRanges.mergeTouching().subtract(global.ranges);
- this.prevSyncComplete = addedRanges.union(MERGE_ADJACENT,
prevSyncComplete.subtract(removedRanges));
- this.curSyncComplete = this.syncComplete = addedRanges;
+ this.syncComplete = addedRanges;
Review Comment:
Could you explain why we do not need to use reemoved ranges anymore?..
##########
accord-core/src/main/java/accord/local/Node.java:
##########
@@ -241,7 +242,21 @@ public long epoch()
private synchronized EpochReady onTopologyUpdateInternal(Topology
topology, boolean startSync)
{
Supplier<EpochReady> bootstrap = commandStores.updateTopology(this,
topology, startSync);
- return this.topology.onTopologyUpdate(topology, bootstrap);
+ Supplier<EpochReady> ordering = () -> {
+ if (this.topology.isEmpty()) return bootstrap.get();
+ return order(this.topology.epochReady(topology.epoch() - 1),
bootstrap.get());
+ };
+ return this.topology.onTopologyUpdate(topology, ordering);
+ }
+
+ private static EpochReady order(EpochReady previous, EpochReady next)
Review Comment:
Should we add an assert that `prev` and `next` are immediately subsequent?
ie epochs diff is 1
--
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]
