[GitHub] [cassandra-accord] belliottsmith commented on a diff in pull request #49: Bootstrap tcm integration

["belliottsmith (via GitHub)" <gi...@apache.org>]

belliottsmith commented on code in PR #49:
URL: https://github.com/apache/cassandra-accord/pull/49#discussion_r1228590771


##########
accord-core/src/main/java/accord/impl/InMemoryCommandStore.java:
##########
@@ -461,6 +461,19 @@ void update(Ranges add)
         }
     }
 
+    public AsyncChain<Timestamp> maxAppliedFor(Seekables<?, ?> keysOrRanges, Ranges slice)
+    {
+        Seekables<?, ?> sliced = keysOrRanges.slice(slice, Minimal);
+        Timestamp timestamp = Timestamp.NONE;
+        for (GlobalCommand globalCommand : commands.values())

Review Comment:
   I think this is actually potentially a bug. Range transactions take the place of key transactions on aggregate, including for the purpose of bootstrap. Let's say you bootstrap from node A to node B, then from node B to node C, and let's say you don't receive any new transactions for some range in between. You'll have a maxApplied of NONE, rather than the range transaction that brought data from A to B.
   
   I'm not sure if this would in practice lead to any isolation violations, but we probably do want to include range transactions as they (pretty much only) provide ordering guarantees with respect to other transactions right now.



-- 
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: pr-unsubscribe@cassandra.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: pr-unsubscribe@cassandra.apache.org
For additional commands, e-mail: pr-help@cassandra.apache.org