dcapwell commented on code in PR #33:
URL: https://github.com/apache/cassandra-accord/pull/33#discussion_r1123900624
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -207,26 +215,30 @@ else if (failure != null)
private void ack()
{
+ // wait for -1 because to ensure the setup phase has also completed.
Setup calls ack in its callback
Review Comment:
```suggestion
// wait for -1 to ensure the setup phase has also completed. Setup
calls ack in its callback
```
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -207,26 +215,30 @@ else if (failure != null)
private void ack()
{
+ // wait for -1 because to ensure the setup phase has also completed.
Setup calls ack in its callback
+ // and prevents races where we respond before dispatching all the
required reads (if the reads are
+ // completing faster than the reads can be setup on all required
shards)
if (-1 == --waitingOnCount)
node.reply(replyTo, replyContext, new ReadOk(data));
}
private synchronized void readComplete(CommandStore commandStore, Data
result)
{
- Preconditions.checkState(waitingOn.get(commandStore.id()));
+ Invariants.checkState(waitingOn.get(commandStore.id()));
logger.trace("{}: read completed on {}", txnId, commandStore);
if (result != null)
data = data == null ? result : data.merge(result);
+ Invariants.checkState(waitingOn.get(commandStore.id()));
Review Comment:
can we get a error msg? if this fails in prod it can be hard to know was
this pre or post `data.merge`, though I don't understand the case this happens.
We only remove in this method and we only mutate the BitSet around
`synchronized` blocks, so its not clear how this condition could fail after the
first was success
##########
accord-core/src/main/java/accord/messages/Commit.java:
##########
@@ -174,6 +178,11 @@ public ReadNack reduce(ReadNack r1, ReadNack r2)
@Override
public void accept(ReadNack reply, Throwable failure)
{
+ if (failure != null)
+ {
+ logger.error("Unhandled exception during commit", failure);
Review Comment:
should we not give this to the agent as it isn't handled?
```suggestion
node.agent().onUncaughtException(failure); //TODO would be nice
to provide "Unhandled exception during commit" for added context
```
--
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]
