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


##########
accord-core/src/main/java/accord/local/durability/ShardDurability.java:
##########
@@ -346,6 +346,9 @@ synchronized void retry(Ranges ranges)
 
         synchronized void start()
         {
+            if (!isStarted())

Review Comment:
   Just for readability, could you maybe make it `if 
(!ShardDurability.this.isStarted())` ? Probably superfluous, but I think it 
might be useful to disambiguate. 



##########
accord-core/src/main/java/accord/local/CommandSummaries.java:
##########
@@ -60,185 +62,279 @@ enum SummaryStatus
         INVALIDATED;
 
         public static final SummaryStatus NONE = null;
+
+        private static final SummaryStatus[] SUMMARY_STATUSES = values();
     }
 
-    enum IsDep { IS_COORD_DEP, IS_NOT_COORD_DEP, NOT_ELIGIBLE, IS_STABLE_DEP, 
IS_NOT_STABLE_DEP }
+    enum IsDep
+    {
+        IS_COORD_DEP, IS_NOT_COORD_DEP, NOT_ELIGIBLE, IS_STABLE_DEP, 
IS_NOT_STABLE_DEP;
+        private static final IsDep[] IS_DEPS = values();
+    }
 
-    class Summary
+    class Summary extends TxnId
     {
-        public final @Nonnull TxnId txnId;
-        public final @Nonnull Timestamp executeAt;
-        public final @Nonnull SummaryStatus status;
-        public final @Nonnull Unseekables<?> participants;
+        private static final int SUMMARY_STATUS_MASK = 0x7;
+        private static final int IS_DEP_SHIFT = 3;
+        final @Nonnull Timestamp executeAt;
+        final int encoded;
+        final Unseekables<?> participants;
 
-        public final IsDep dep;
-        public final TxnId findAsDep;
+        public Summary slice(Ranges ranges)
+        {
+            return new Summary(this, this.executeAt, encoded, 
participants.slice(ranges, Minimal));
+        }
 
         @VisibleForTesting
-        public Summary(@Nonnull TxnId txnId, @Nonnull Timestamp executeAt, 
@Nonnull SummaryStatus status, @Nonnull Unseekables<?> participants, IsDep dep, 
TxnId findAsDep)
+        public Summary(@Nonnull TxnId txnId, @Nonnull Timestamp executeAt, 
@Nonnull SummaryStatus status, IsDep dep, Unseekables<?> participants)
         {
-            this.txnId = txnId;
-            this.executeAt = executeAt;
-            this.status = status;
+            super(txnId);
             this.participants = participants;
-            this.findAsDep = findAsDep;
-            this.dep = dep;
+            this.executeAt = executeAt.equals(txnId) ? this : executeAt;
+            this.encoded = status.ordinal() | (dep == null ? Integer.MIN_VALUE 
: (dep.ordinal() << IS_DEP_SHIFT));
+        }
+
+        private Summary(@Nonnull TxnId txnId, @Nonnull Timestamp executeAt, 
int encoded, Unseekables<?> participants)
+        {
+            super(txnId);
+            this.participants = participants;
+            this.executeAt = executeAt == txnId || executeAt.equals(txnId) ? 
this : executeAt;
+            this.encoded = encoded;
+        }
+
+        public boolean is(IsDep isDep)
+        {
+            return (encoded >> IS_DEP_SHIFT) == isDep.ordinal();
         }
 
-        public Summary slice(Ranges slice)
+        public IsDep isDep()
         {
-            return new Summary(txnId, executeAt, status, 
participants.slice(slice, Minimal), dep, findAsDep);
+            int ordinal = encoded >> IS_DEP_SHIFT;
+            return ordinal < 0 ? null : IsDep.IS_DEPS[ordinal];
+        }
+
+        public boolean is(SummaryStatus summaryStatus)
+        {
+            return (encoded & SUMMARY_STATUS_MASK) == summaryStatus.ordinal();
+        }
+
+        public SummaryStatus status()
+        {
+            int ordinal = encoded & SUMMARY_STATUS_MASK;
+            return SummaryStatus.SUMMARY_STATUSES[ordinal];
+        }
+
+        public TxnId plainTxnId()
+        {
+            return new TxnId(this);
+        }
+
+        public Timestamp plainExecuteAt()
+        {
+            return executeAt == this ? new Timestamp(this) : executeAt;
         }
 
         @Override
         public String toString()
         {
             return "Summary{" +
-                   "txnId=" + txnId +
-                   ", executeAt=" + executeAt +
-                   ", saveStatus=" + status +
-                   ", participants=" + participants +
-                   ", maybeDep=" + dep +
-                   ", findAsDep=" + findAsDep +
+                   "txnId=" + plainTxnId() +
+                   ", executeAt=" + plainExecuteAt() +
+                   ", saveStatus=" + status() +
+                   ", isDep=" + isDep() +
                    '}';
         }
+    }
 
-        public static class Loader
+    class SummaryLoader
+    {
+        public interface Factory<L extends SummaryLoader, P>
         {
-            public interface Factory<L extends Loader>
-            {
-                L create(@Nullable TxnId primaryTxnId, Unseekables<?> 
searchKeysOrRanges, RedundantBefore redundantBefore, Kinds testKind, TxnId 
minTxnId, Timestamp maxTxnId, @Nullable TxnId findAsDep);
-            }
+            L create(P param, RedundantBefore redundantBefore, @Nullable 
MaxDecidedRX maxDecidedRX, TxnId primaryTxnId, Unseekables<?> 
searchKeysOrRanges, Kinds testKind, TxnId minTxnId, Timestamp maxTxnId, 
@Nullable TxnId findAsDep);
+        }
 
-            protected final Unseekables<?> searchKeysOrRanges;
-            protected final RedundantBefore redundantBefore;
-            // TODO (expected): separate out Kinds we need before/after 
primaryTxnId/executeAt
-            protected final Kinds testKind;
-            protected final TxnId minTxnId;
-            protected final Timestamp maxTxnId;
-            @Nullable protected final TxnId findAsDep;
+        protected final RedundantBefore redundantBefore;
+        protected final MaxDecidedRX maxDecidedRX;
+        protected final Unseekables<?> searchKeysOrRanges;
+        // TODO (expected): separate out Kinds we need before/after 
primaryTxnId/executeAt
+        protected final Kinds testKind;
+        protected final TxnId primaryTxnId, findAsDep, minTxnId, minDecidedId;
+        protected final Timestamp maxTxnId;
+//        protected final TxnId minDecidedId;

Review Comment:
   nit: minDecidedId already added as a field above, can be cleaned up here



##########
accord-core/src/main/java/accord/local/CommandStore.java:
##########
@@ -666,11 +666,33 @@ protected void updatedRedundantBefore(SafeCommandStore 
safeStore, RedundantBefor
         listeners.clearBefore(this, clearWaitingBefore);
     }
 
-    protected void markSynced(SafeCommandStore safeStore, TxnId syncId, Ranges 
ranges)
+    protected final boolean isWaitingOnSync(TxnId syncId, Ranges ranges)
+    {
+        if (waitingOnSync.isEmpty())
+            return false;
+
+        for (Map.Entry<Long, WaitingOnSync> e : waitingOnSync.entrySet())
+        {
+            if (e.getKey() > syncId.epoch())
+                break;
+
+            Ranges remaining = e.getValue().ranges;
+            boolean intersects = remaining.intersects(ranges);
+            if (intersects)
+                return true;
+        }
+
+        return true;

Review Comment:
   Should this return false maybe, since we didn't get any intersection matches 
that would return true?



##########
accord-core/src/main/java/accord/local/Bootstrap.java:
##########
@@ -135,20 +138,24 @@ void start(SafeCommandStore safeStore)
             // of these ranges as part of this attempt
             Ranges commitRanges = valid;
             safeStore = safeStore;
-            // we submit a separate execution so that we know 
markBootstrapping is durable before we initiate the fetch
-            safeStore.commandStore()
-                     .build((PreLoadContext.Empty) () -> "Start Bootstrap RX", 
safeStore0 -> {
-                         store.markBootstrapping(safeStore0, globalSyncId, 
commitRanges);
-                         return CoordinateSyncPoint.exclusive(node, 
globalSyncId, commitRanges);
-                     })
-                     .flatMap(i -> i)
-                     .flatMap(syncPoint -> node.withEpochAtLeast(epoch, null, 
() -> store.build((PreLoadContext.Empty) () -> "Start Bootstrap Fetch", 
safeStore1 -> {
-                         if (valid.isEmpty()) // we've lost ownership of the 
range
-                             return AsyncResults.success(Ranges.EMPTY);
-                         return fetch = safeStore1.dataStore().fetch(node, 
safeStore1, valid, syncPoint, this);
-                     })))
-                     .flatMap(i -> i)
-                     .begin(this);
+            CommandStore commandStore = safeStore.commandStore();
+            node.durability()
+                // we first make sure the sync point is durable to a majority, 
since any later durability conditions
+                // this node participates in will not guarantee a quorum for 
preceding transactions
+                .sync("Bootstrap " + commitRanges + " for " + 
safeStore.commandStore(), globalSyncId, commitRanges, NoLocal, Quorum, 1L, 
TimeUnit.HOURS)
+                .flatMap(success -> commandStore.build((PreLoadContext.Empty) 
() -> "Start Bootstrap RX", safeStore0 -> {
+                    // we submit a separate execution so that we know 
markBootstrapping is durable before we initiate the fetch
+                    store.markBootstrapping(safeStore0, globalSyncId, 
commitRanges);

Review Comment:
   Do we maybe want to mark bootstrapping before we wait for durability 
majority? This used to be the first step we were making previously if I 
understand the left hand side correctly.



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