ifesdjeen commented on code in PR #150:
URL: https://github.com/apache/cassandra-accord/pull/150#discussion_r1884292590


##########
accord-core/src/main/java/accord/impl/AbstractConfigurationService.java:
##########
@@ -289,13 +289,16 @@ public void fetchTopologyForEpoch(long epoch)
         fetchTopologyInternal(epoch);
     }
 
+    // TODO (expected): rename, sync is too ambiguous

Review Comment:
   Oh yes please. I was looking into whether we need to move this into Listener 
because of this name, too. 



##########
accord-core/src/main/java/accord/local/RedundantBefore.java:
##########
@@ -712,37 +857,95 @@ public Ranges removePreBootstrap(TxnId txnId, Ranges 
ranges)
     {
         if (maxBootstrap.compareTo(txnId) <= 0)
             return ranges;
-        return foldl(ranges, Entry::removePreBootstrap, ranges, txnId, null, r 
-> false);
+        return foldl(ranges, Entry::withoutPreBootstrap, ranges, txnId, null, 
r -> false);
     }
 
     /**
-     * Subtract any ranges we consider stale or pre-bootstrap
+     * Subtract anything we don't need to coordinate (because they are known 
to be shard durable),
+     * and we don't execute locally, i.e. are pre-bootstrap or stale (or for 
RX are on ranges that are already retired)
      */
-    public Ranges expectToExecute(TxnId txnId, @Nonnull Timestamp executeAt, 
Ranges ranges)
+    public Participants<?> expectToOwn(TxnId txnId, @Nullable EpochSupplier 
executeAt, Participants<?> participants)
     {
-        Invariants.checkArgument(executeAt != null, "executeAt must not be 
null");
-        if (maxBootstrap.compareTo(txnId) <= 0 && (staleRanges == null || 
!staleRanges.intersects(ranges)))
-            return ranges;
-        return foldl(ranges, Entry::expectToExecute, ranges, txnId, executeAt, 
r -> false);
+        if (txnId.is(ExclusiveSyncPoint))
+        {
+            if (!mayFilterStaleOrPreBootstrapOrRetired(txnId, participants))
+                return participants;
+
+            return foldl(participants, 
Entry::withoutRedundantAnd_StaleOrPreBootstrapOrRetired, participants, txnId, i 
-> false);
+        }
+        else
+        {
+            if (!mayFilterStaleOrPreBootstrap(txnId, participants))
+                return participants;
+
+            return foldl(participants, 
Entry::withoutRedundantAnd_StaleOrPreBootstrap, participants, txnId, executeAt, 
r -> false);
+        }
     }
 
     /**
-     * Subtract any ranges we consider stale or pre-bootstrap at any point
+     * Subtract anything we won't execute locally, i.e. are pre-bootstrap or 
stale (or for RX are on ranges that are already retired)
      */
-    public Ranges everExpectToExecute(TxnId txnId, Ranges ranges)
+    public Participants<?> expectToExecute(TxnId txnId, @Nullable 
EpochSupplier executeAt, Participants<?> participants)
     {
-        if (maxBootstrap.compareTo(txnId) <= 0 && (staleRanges == null || 
!staleRanges.intersects(ranges)))
-            return ranges;
-        return foldl(ranges, Entry::expectToExecute, ranges, txnId, null, r -> 
false);
+        if (txnId.is(ExclusiveSyncPoint))
+        {
+            if (!mayFilterStaleOrPreBootstrapOrRetired(txnId, participants))
+                return participants;
+
+            return foldl(participants, 
Entry::withoutStaleOrPreBootstrapOrLocallyRetired, participants, txnId, i -> 
false);
+        }
+        else
+        {
+            if (!mayFilterStaleOrPreBootstrap(txnId, participants))
+                return participants;
+
+            return foldl(participants, 
Entry::participantsWithoutStaleOrPreBootstrap, participants, txnId, executeAt, 
r -> false);
+        }
+    }
+
+    public boolean mayFilter(TxnId txnId, Participants<?> participants)

Review Comment:
   nit: why a shortcut name for this one?



##########
accord-core/src/main/java/accord/local/cfk/Pruning.java:
##########
@@ -393,23 +393,23 @@ private static Long2ObjectHashMap<TxnInfo> 
buildEpochPrunedBefores(TxnInfo[] byI
             return null;
 
         Long2ObjectHashMap<TxnInfo> epochPrunedBefores = new 
Long2ObjectHashMap<>();
-        for (long epoch = newPrunedBefore.epoch() ; epoch <= 
newPrunedBefore.executeAt.epoch(); ++epoch)
-            epochPrunedBefores.put(epoch, newPrunedBefore);
+        epochPrunedBefores.put(newPrunedBefore.epoch(), newPrunedBefore);
 
-        int maxi = Arrays.binarySearch(committedByExecuteAt, newPrunedBefore, 
TxnInfo::compareExecuteAt);
+        int maxi = -1 - Arrays.binarySearch(committedByExecuteAt, 
newPrunedBefore, (a, b) -> a.compareExecuteAtEpoch(b) >= 0 ? 1 : -1);

Review Comment:
   do we need to check for < 0 here?



##########
accord-core/src/main/java/accord/coordinate/tracking/QuorumIdTracker.java:
##########
@@ -0,0 +1,122 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package accord.coordinate.tracking;
+
+import java.util.Set;
+
+import accord.local.Node;
+import accord.topology.Shard;
+import accord.topology.Topologies;
+import org.agrona.collections.ObjectHashSet;
+
+import static accord.coordinate.tracking.AbstractTracker.ShardOutcomes.Fail;
+import static 
accord.coordinate.tracking.AbstractTracker.ShardOutcomes.NoChange;
+import static accord.coordinate.tracking.AbstractTracker.ShardOutcomes.Success;
+
+public class QuorumIdTracker extends 
SimpleTracker<QuorumIdTracker.QuorumIdShardTracker> implements ResponseTracker

Review Comment:
   Should we maybe always use `QuorumIdTracker`? I have been in a situation 
recently, where I had to reconstruct IDs of successes and failures.



##########
accord-core/src/main/java/accord/local/Commands.java:
##########
@@ -91,7 +102,7 @@ private Commands()
     {
     }
 
-    public enum AcceptOutcome { Success, Redundant, RejectedBallot, Truncated }
+    public enum AcceptOutcome { Success, Redundant, RejectedBallot, Retired, 
Truncated }

Review Comment:
   Is this a correctness issue or optimizaiton? In either case would be great 
to document.



##########
accord-core/src/main/java/accord/local/RedundantBefore.java:
##########
@@ -161,8 +185,7 @@ public Entry(Range range, long startOwnershipEpoch, long 
endOwnershipEpoch, @Non
             this.bootstrappedAt = bootstrappedAt;
             this.staleUntilAtLeast = staleUntilAtLeast;
             checkNoneOrRX(locallyWitnessedOrInvalidatedBefore, 
locallyAppliedOrInvalidatedBefore, locallyDecidedAndAppliedOrInvalidatedBefore,
-                          shardAppliedOrInvalidatedBefore, gcBefore);
-            checkPartiallyOrdered(locallyDecidedAndAppliedOrInvalidatedBefore, 
locallyAppliedOrInvalidatedBefore);
+                          shardAppliedOrInvalidatedBefore, 
gcBefore);checkPartiallyOrdered(locallyDecidedAndAppliedOrInvalidatedBefore, 
locallyAppliedOrInvalidatedBefore);

Review Comment:
   nit: line break here?



##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -331,6 +332,17 @@ protected void updatedRedundantBefore(SafeCommandStore 
safeStore, TxnId syncId,
                 }
             });
         });
+        TxnId clearProgressLogBefore = 
unsafeGetRedundantBefore().minShardRedundantBefore();
+        List<TxnId> clearing = ((DefaultProgressLog) 
progressLog).activeBefore(clearProgressLogBefore);
+        for (TxnId txnId : clearing)
+        {
+            GlobalCommand globalCommand = commands.get(txnId);
+;            Invariants.checkState(globalCommand != null && 
!globalCommand.isEmpty());

Review Comment:
   nit: `;` 



##########
accord-core/src/main/java/accord/local/Commands.java:
##########
@@ -718,7 +706,7 @@ else if (safeStore.isFullyPreBootstrapOrStale(dependency, 
waiting.partialDeps().
     static void updateDependencyAndMaybeExecute(SafeCommandStore safeStore, 
SafeCommand safeCommand, SafeCommand predecessor, boolean notifyWaitingOn)
     {
         Command.Committed command = safeCommand.current().asCommitted();
-        if (command.hasBeen(Applied))
+        if (command.hasBeen(Status.Applied))

Review Comment:
   nit: `Status` added by accident? 



##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -479,11 +479,12 @@ final Supplier<EpochReady> bootstrapper(Node node, Ranges 
newRanges, long epoch)
                 Bootstrap bootstrap = new Bootstrap(node, this, epoch, 
newRanges);
                 bootstraps.add(bootstrap);
                 bootstrap.start(safeStore);
-                return new EpochReady(epoch, null, bootstrap.coordination, 
bootstrap.data, bootstrap.reads);
+                return new EpochReady(epoch, null, null, bootstrap.data, 
bootstrap.reads);

Review Comment:
   I might be missing something, but if the node can process reads, it should 
be ready for coordination, too or?



##########
accord-core/src/main/java/accord/coordinate/ExecuteTxn.java:
##########
@@ -104,7 +106,7 @@ private Commit.Kind commitKind()
     @Override
     public void contact(Id to)
     {
-        if (SEND_MINIMUM_STABLE_MESSAGES) Commit.stableAndRead(to, node, 
allTopologies, commitKind(), txnId, txn, route, readScope, executeAt, 
stableDeps, this, SEND_MINIMUM_STABLE_MESSAGES);
+        if (SEND_MINIMUM_STABLE_MESSAGES && path != RECOVER) 
Commit.stableAndRead(to, node, allTopologies, commitKind(), txnId, txn, route, 
readScope, executeAt, stableDeps, this, false);

Review Comment:
   Is it intended that we e passing `false` here? Above we seem to be passing 
`true` under same condition (send minimum and path not recover).



##########
accord-core/src/main/java/accord/coordinate/CollectLatestDeps.java:
##########
@@ -0,0 +1,126 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package accord.coordinate;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.function.BiConsumer;
+
+import accord.api.RoutingKey;
+import accord.coordinate.tracking.QuorumTracker;
+import accord.local.CommandStore;
+import accord.local.Node;
+import accord.local.Node.Id;
+import accord.messages.Callback;
+import accord.messages.GetLatestDeps;
+import accord.messages.GetLatestDeps.GetLatestDepsOk;
+import accord.primitives.FullRoute;
+import accord.primitives.LatestDeps;
+import accord.primitives.Timestamp;
+import accord.primitives.TxnId;
+import accord.primitives.Unseekables;
+import accord.topology.Topologies;
+import accord.utils.SortedArrays.SortedArrayList;
+import accord.utils.SortedListMap;
+
+import static accord.coordinate.tracking.RequestStatus.Failed;
+import static accord.coordinate.tracking.RequestStatus.Success;
+
+public class CollectLatestDeps implements Callback<GetLatestDepsOk>
+{
+    final Node node;
+    final TxnId txnId;
+    final RoutingKey homeKey;
+    final Timestamp executeAt;
+
+    private final SortedListMap<Id, GetLatestDepsOk> oks;

Review Comment:
   could you elaborate why it is important for this to be sorted?



##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -498,15 +499,21 @@ final Supplier<EpochReady> bootstrapper(Node node, Ranges 
newRanges, long epoch)
     protected Supplier<EpochReady> sync(Node node, Ranges ranges, long epoch)
     {
         return () -> {
-            if (redundantBefore.min(ranges, 
RedundantBefore.Entry::locallyWitnessedBefore).epoch() >= epoch)
-                return new EpochReady(epoch, DONE, DONE, DONE, DONE);
-
-            AsyncResults.SettableResult<Void> whenDone = new 
AsyncResults.SettableResult<>();
-            waitingOnSync.put(epoch, new WaitingOnSync(whenDone, ranges));
-            return new EpochReady(epoch, DONE, whenDone, DONE, DONE);
+            AsyncResult<Void> readyToCoordinate = readyToCoordinate(ranges, 
epoch);
+            return new EpochReady(epoch, DONE, readyToCoordinate, DONE, DONE);

Review Comment:
   hm, looks like I was wrong above (can serve reads -> can coordinate), so it 
would be nice to have this documented somewhere.



##########
accord-core/src/main/java/accord/local/Commands.java:
##########
@@ -107,11 +118,11 @@ public static AcceptOutcome recover(SafeCommandStore 
safeStore, SafeCommand safe
     private static AcceptOutcome preacceptOrRecover(SafeCommandStore 
safeStore, SafeCommand safeCommand, StoreParticipants participants, TxnId 
txnId, long acceptEpoch, PartialTxn partialTxn, FullRoute<?> route, Ballot 
ballot)
     {
         final Command command = safeCommand.current();
-
         if (command.hasBeen(Truncated))
         {
             logger.trace("{}: skipping preaccept - command is truncated", 
txnId);
-            return command.is(Invalidated) ? AcceptOutcome.RejectedBallot : 
AcceptOutcome.Truncated;
+            return command.is(Invalidated) ? AcceptOutcome.RejectedBallot : 
participants.owns().isEmpty()

Review Comment:
   nit: would you mind to expand double ternary?



##########
accord-core/src/main/java/accord/local/Commands.java:
##########
@@ -598,9 +582,11 @@ public static boolean maybeExecute(SafeCommandStore 
safeStore, SafeCommand safeC
         {
             case Stable:
                 // TODO (desirable, efficiency): maintain distinct ReadyToRead 
and ReadyToWrite states
-                // TODO (required): we can have dangling transactions in some 
cases when proposing in a future epoch but
-                //   later deciding on an earlier epoch. We should probably 
turn this into an erased vestigial command,
-                //   but we should tighten up our semantics there in general.
+                // TODO (required): we can have dangling transactions if we 
don't execute them,
+                //  but we don't want to erase the transaction until

Review Comment:
   Sentence seems to be finished abruptly?



##########
accord-core/src/main/java/accord/local/RedundantBefore.java:
##########
@@ -712,37 +857,95 @@ public Ranges removePreBootstrap(TxnId txnId, Ranges 
ranges)
     {
         if (maxBootstrap.compareTo(txnId) <= 0)
             return ranges;
-        return foldl(ranges, Entry::removePreBootstrap, ranges, txnId, null, r 
-> false);
+        return foldl(ranges, Entry::withoutPreBootstrap, ranges, txnId, null, 
r -> false);
     }
 
     /**
-     * Subtract any ranges we consider stale or pre-bootstrap
+     * Subtract anything we don't need to coordinate (because they are known 
to be shard durable),
+     * and we don't execute locally, i.e. are pre-bootstrap or stale (or for 
RX are on ranges that are already retired)
      */
-    public Ranges expectToExecute(TxnId txnId, @Nonnull Timestamp executeAt, 
Ranges ranges)
+    public Participants<?> expectToOwn(TxnId txnId, @Nullable EpochSupplier 
executeAt, Participants<?> participants)
     {
-        Invariants.checkArgument(executeAt != null, "executeAt must not be 
null");
-        if (maxBootstrap.compareTo(txnId) <= 0 && (staleRanges == null || 
!staleRanges.intersects(ranges)))
-            return ranges;
-        return foldl(ranges, Entry::expectToExecute, ranges, txnId, executeAt, 
r -> false);
+        if (txnId.is(ExclusiveSyncPoint))
+        {
+            if (!mayFilterStaleOrPreBootstrapOrRetired(txnId, participants))
+                return participants;
+
+            return foldl(participants, 
Entry::withoutRedundantAnd_StaleOrPreBootstrapOrRetired, participants, txnId, i 
-> false);
+        }
+        else
+        {
+            if (!mayFilterStaleOrPreBootstrap(txnId, participants))
+                return participants;
+
+            return foldl(participants, 
Entry::withoutRedundantAnd_StaleOrPreBootstrap, participants, txnId, executeAt, 
r -> false);
+        }
     }
 
     /**
-     * Subtract any ranges we consider stale or pre-bootstrap at any point
+     * Subtract anything we won't execute locally, i.e. are pre-bootstrap or 
stale (or for RX are on ranges that are already retired)
      */
-    public Ranges everExpectToExecute(TxnId txnId, Ranges ranges)
+    public Participants<?> expectToExecute(TxnId txnId, @Nullable 
EpochSupplier executeAt, Participants<?> participants)
     {
-        if (maxBootstrap.compareTo(txnId) <= 0 && (staleRanges == null || 
!staleRanges.intersects(ranges)))
-            return ranges;
-        return foldl(ranges, Entry::expectToExecute, ranges, txnId, null, r -> 
false);
+        if (txnId.is(ExclusiveSyncPoint))
+        {
+            if (!mayFilterStaleOrPreBootstrapOrRetired(txnId, participants))
+                return participants;
+
+            return foldl(participants, 
Entry::withoutStaleOrPreBootstrapOrLocallyRetired, participants, txnId, i -> 
false);
+        }
+        else
+        {
+            if (!mayFilterStaleOrPreBootstrap(txnId, participants))

Review Comment:
   nit: do I understand right that we can get here only for non-exclusive 
syncpoints?



##########
accord-core/src/main/java/accord/local/Commands.java:
##########
@@ -1162,41 +1246,27 @@ static Command 
removeNoLongerOwnedDependency(SafeCommandStore safeStore, SafeCom
         return safeCommand.updateWaitingOn(update);
     }
 
-    /**
-     * A key nominated to represent the "home" shard - only members of the 
home shard may be nominated to recover
-     * a transaction, to reduce the cluster-wide overhead of ensuring 
progress. A transaction that has only been
-     * witnessed at PreAccept may however trigger a process of ensuring the 
home shard is durably informed of
-     * the transaction.
-     *
-     * Note that for ProgressLog purposes the "home shard" is the shard as of 
txnId.epoch.
-     * For recovery purposes the "home shard" is as of txnId.epoch until 
Committed, and executeAt.epoch once Executed
-     */
-    @SuppressWarnings({"unchecked", "rawtypes"})
-    public static CommonAttributes updateParticipants(Command command, 
StoreParticipants participants)
-    {
-        participants = command.participants().supplement(participants);
+    public static CommonAttributes supplementParticipants(Command command, 
StoreParticipants participants)
+    {   // TODO (required): invoke participants.supplement before creating 
mutable
         if (command.participants() == participants)
             return command;
 
-        return command.mutable().updateParticipants(participants);
+        return command.mutable().supplementParticipants(participants);
     }
 
     public static CommonAttributes updateRoute(Command command, Route<?> route)
     {
         StoreParticipants participants = 
command.participants().supplement(route);
-        if (command.participants() == participants)
-            return command;
-
-        return command.mutable().updateParticipants(participants);
+        return supplementParticipants(command, participants);
     }
 
-    public static Command updateParticipants(SafeCommandStore safeStore, 
SafeCommand safeCommand, StoreParticipants participants)
+    public static Command supplementParticipants(SafeCommandStore safeStore, 
SafeCommand safeCommand, StoreParticipants participants)
     {
         Command current = safeCommand.current();
         if (current.saveStatus().compareTo(Erased) >= 0)
             return current;
-
-        CommonAttributes updated = updateParticipants(current, participants);
+        // TODO (required): invoke updateParticipants directly

Review Comment:
   nit: supplement?



##########
accord-core/src/main/java/accord/local/RedundantStatus.java:
##########
@@ -80,6 +92,26 @@ public enum RedundantStatus
      */
     LOCALLY_REDUNDANT,
 
+    /**
+     * The relevant owned ranges are all shard-redundant AND (pre-bootstrap or 
stale), meaning any intersecting transaction

Review Comment:
   Are these new states required for correctness? 



##########
accord-core/src/main/java/accord/local/cfk/CommandsForKey.java:
##########
@@ -740,7 +753,7 @@ public enum InternalStatus
         STABLE(true, false),
         APPLIED(true, false),
         INVALIDATED(false, false),
-        TRUNCATED_OR_PRUNED(false, false)

Review Comment:
   could you elaborate a difference between truncated and pruned here? 



##########
accord-core/src/main/java/accord/local/cfk/PostProcess.java:
##########
@@ -275,30 +275,54 @@ static NotifyUnmanagedResult notifyUnmanaged(Unmanaged[] 
unmanageds,
         }
 
         {
-            Timestamp applyTo = null;
             if (newInfo != null && newInfo.is(APPLIED))
             {
                 TxnInfo maxContiguousApplied = 
CommandsForKey.maxContiguousManagedApplied(committedByExecuteAt, 
maxAppliedWriteByExecuteAt, bootstrappedAt);
                 if (maxContiguousApplied != null && 
maxContiguousApplied.compareExecuteAt(newInfo) >= 0)
-                    applyTo = maxContiguousApplied.executeAt;
-            }
-            else if (newInfo == null)
-            {
-                TxnInfo maxContiguousApplied = 
CommandsForKey.maxContiguousManagedApplied(committedByExecuteAt, 
maxAppliedWriteByExecuteAt, bootstrappedAt);
-                if (maxContiguousApplied != null)
-                    applyTo = maxContiguousApplied.executeAt;
-
-                applyTo = Timestamp.nonNullOrMax(applyTo, 
TxnId.nonNullOrMax(redundantBefore, bootstrappedAt));
+                {
+                    Timestamp applyTo = maxContiguousApplied.executeAt;
+                    int start = findFirstApply(unmanageds);
+                    int end = findApply(unmanageds, start, applyTo);
+                    if (start != end)
+                    {
+                        TxnId[] notifyNotWaiting = selectUnmanaged(unmanageds, 
start, end);
+                        unmanageds = removeUnmanaged(unmanageds, start, end);
+                        notifier = new PostProcess.NotifyNotWaiting(notifier, 
notifyNotWaiting);
+                    }
+                }
             }
-
-            if (applyTo != null)
+            else if (newInfo == null && isNewBoundsInfo)
             {
                 int start = findFirstApply(unmanageds);
-                int end = findApply(unmanageds, start, applyTo);
-                if (start != end)
+                int end = start;
+                int j = 1 + 
maxContiguousManagedAppliedIndex(committedByExecuteAt, 
maxAppliedWriteByExecuteAt, bootstrappedAt);
+                while (end < unmanageds.length && j < 
committedByExecuteAt.length)
+                {
+                    int c = 
committedByExecuteAt[j].executeAt.compareTo(unmanageds[end].waitingUntil);
+                    if (c == 0)
+                    {
+                        if (start != end)
+                        {
+                            TxnId[] notifyNotWaiting = 
selectUnmanaged(unmanageds, start, end);
+                            unmanageds = removeUnmanaged(unmanageds, start, 
end);
+                            end -= (end - start);

Review Comment:
   Had to re-read this several times. Isn't this just `end = start` ?



##########
accord-core/src/main/java/accord/messages/AbstractRequest.java:
##########
@@ -81,7 +81,7 @@ public final void process(Node on, Node.Id replyTo, 
ReplyContext replyContext)
         {
             long expiresAt = node.agent().expiresAt(replyContext, 
MICROSECONDS);
             if (expiresAt > 0)
-            {
+            {   // TODO (required): now!! this should be registerAt (not 
changing to avoid messing up seed)

Review Comment:
   Reminding of this one



##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -53,12 +53,15 @@
 import static accord.api.ProgressLog.BlockedUntil.HasStableDeps;
 import static accord.messages.MessageType.READ_RSP;
 import static accord.messages.ReadData.CommitOrReadNack.Insufficient;
+import static accord.messages.ReadData.CommitOrReadNack.Invalid;
 import static accord.messages.ReadData.CommitOrReadNack.Redundant;
 import static accord.messages.TxnRequest.latestRelevantEpochIndex;
 import static accord.primitives.Routables.Slice.Minimal;
 import static accord.utils.Invariants.illegalState;
 import static java.util.concurrent.TimeUnit.MICROSECONDS;
 
+// TODO (expected): if one shard timesout waiting to reply, but another shard 
produces a reply, return a partial response (or response with suitably 
populated unavailable)

Review Comment:
   nit: times out



##########
accord-core/src/main/java/accord/messages/Commit.java:
##########
@@ -173,64 +174,65 @@ public static void stableAndRead(Node node, Topologies 
all, Kind kind, TxnId txn
     {
         Invariants.checkState(all.oldestEpoch() == txnId.epoch());
         Invariants.checkState(all.currentEpoch() == executeAt.epoch());
-        Topology executes = all.forEpoch(executeAt.epoch());
-        Topology coordinates = all.forEpoch(txnId.epoch());
+        Topology executes = all.getEpoch(executeAt.epoch());
+        Topology coordinates = all.getEpoch(txnId.epoch());
 
         SortedArrayList<Id> contact = all.nodes().without(all::isFaulty);
-        sendTo(contact, readSet, (set, id) -> set.contains(id.id), readScope, 
node, coordinates, executes, all, kind, Ballot.ZERO,
+        sendTo(contact, readSet, (set, id) -> set.contains(id.id), (set, id) 
-> false, readScope, node, coordinates, executes, all, kind, Ballot.ZERO,

Review Comment:
   Did you intend to add this _after_ `(set, id) -> set.contains(id.id)`? 



-- 
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]

Reply via email to