[GitHub] [cassandra-accord] aweisberg commented on a diff in pull request #40: CASSANDRA-18422: when status changes after read result, reject

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

aweisberg commented on code in PR #40:
URL: https://github.com/apache/cassandra-accord/pull/40#discussion_r1160856957


##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -53,7 +54,35 @@ public static ReadData create(TxnId txnId, Seekables<?, ?> scope, long executeAt
             return new ReadData(txnId, scope, executeAtEpoch, waitForEpoch);
         }
     }
+    private class ObsoleteTracker implements CommandListener

Review Comment:
   Instead of allocating a separate obsolete tracker could it be done in the ReadData `onChange` method?
   
   It seems like the obsolete field is concurrently read/written by multiple command stores and might end up sending multiple replies back?
   
   Would it be better/simpler to mark it as obsolete then ack the local map reduce so that the existing code that waits to hear from all the command stores runs as normal?
   
   I don't understand the threading of this in general since command listeners run on arbitrary threads at arbitrary times?
   
   It seems like `apply` takes care to be synchronized as does `onChange`



##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -53,7 +54,35 @@ public static ReadData create(TxnId txnId, Seekables<?, ?> scope, long executeAt
             return new ReadData(txnId, scope, executeAtEpoch, waitForEpoch);
         }
     }
+    private class ObsoleteTracker implements CommandListener
+    {
+        @Override
+        public void onChange(SafeCommandStore safeStore, SafeCommand safeCommand)
+        {
+            switch (safeCommand.current().status())

Review Comment:
   This doesn't check whether the read already successfully completed before the transition to `PreApplied` or `Applied` for this command store. It may be that it wasn't obsoleted.



##########
accord-core/src/test/java/accord/burn/BurnTest.java:
##########
@@ -324,6 +334,7 @@ public static void main(String[] args)
     @Test
     public void testOne()
     {
+        for (; true; )

Review Comment:
   Is this supposed to be here still?



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