dcapwell commented on code in PR #40:
URL: https://github.com/apache/cassandra-accord/pull/40#discussion_r1160895667
##########
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?
we would then require special handle to avoid dup calls to `read`. If we
see the status `ReadyToExecute` twice (due to something else changing) then we
would trigger `read` another time. By removing the listener and adding a new
one, we avoid this
> It seems like the obsolete field is concurrently read/written by multiple
command stores
read by different stores is fine. write is guarded by
`accord.messages.ReadData#obsolete` but that isn't sync, so we could have
duplicates if 2 stores try to call at the same time; I can add a test for this
to make sure its a single reply, though don't think I can actually force this
timing event to happen... so not 100% sure I can test this... but I can at
least have both stores fail in a test... good to do anyways
ATM duplicate replies is fine as the second one is ignored, but better to
optimize it away.
> Would it be better/simpler to mark it as obsolete then ack the local map
reduce
the MapReduce has a state where it doesn't know the result as it relies on
listeners... so only the case where `apply` marks as `obsolete` can be handled
in MapReduce; which it currently does.
> I don't understand the threading of this in general since command
listeners run on arbitrary threads at arbitrary times?
listeners are per-store and are single threaded. You attach a listener to a
pair of `(command, store)`, and always run in that same `store`.
Now, there is a bug with listeners that is improved in
https://issues.apache.org/jira/browse/CASSANDRA-18364 and solved in a followup
from Benedict... so this logic is currently safe based off expected semantics,
but current patch could produce multiple `read` events.
The bug that is fixed in CASSANDRA-18364 is a recursion bug:
* an event causes a command to trigger listeners
** a listener changes the command, causing listeners to be called
*** listener removes self, so should not be called again
* removed listener is called again as its referenced from an `immutable`
list of listeners...
The last step is fixed in CASSANDRA-18364 by double checking that the
listener still exists and Benedict said he was reworking all this logic.
--
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]
