dcapwell commented on code in PR #56:
URL: https://github.com/apache/cassandra-accord/pull/56#discussion_r1297782565
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -149,14 +149,16 @@ void read(SafeCommandStore safeStore, Timestamp
executeAt, PartialTxn txn)
Ranges unavailable = safeStore.ranges().unsafeToReadAt(executeAt);
txn.read(safeStore, executeAt).begin((next, throwable) -> {
- if (throwable != null)
+ // TODO (expected, exceptions): should send exception to client,
and consistency handle/propagate locally
+ logger.trace("{}: read failed for {}: {}", txnId, unsafeStore,
throwable);
+ synchronized (ReadData.this)
{
- // TODO (expected, exceptions): should send exception to
client, and consistency handle/propagate locally
- logger.trace("{}: read failed for {}: {}", txnId, unsafeStore,
throwable);
- node.reply(replyTo, replyContext, ReadNack.Error);
+ if (fail == null)
+ fail = throwable;
+ else
+ fail.addSuppressed(throwable);
}
- else
- readComplete(unsafeStore, next, unavailable);
+ readComplete(unsafeStore, next, unavailable);
Review Comment:
> I think the interface for listeners, task submission, and callbacks errors
is possibly what is wrong. Every execution should occur with a context that
specifies what the uncaught error handling should be,
I agree with you on this point... AsyncChain and Futures are best when we
"chain" them and centralize the error handling, but when we keep mixing and
matching it gets hard to get right...
the common pattern is
```
// this is actually async, but to simplify the code I didn't do that
A value = null;
for store in stores.intersects(request):
A local = store.process(request.map)
value = request.reduce(value, local);
request.accept(value)
```
and in this case we try to add more state to make sure all `map` are called
and `accept` is called (which is why we use `-1` in `-1 ==
--waitingOnCount`)...
We could change this pattern to be
```
List<AsyncChain<A>> chains;
for store in stores.intersects(request):
chains.add(store.process(request.map));
AsyncChains.reduce(chains, request.reduce)
```
this would allow us to start composing and chaining, rather than stoping
chains, and forking new ones in different places...
Now, given how that is a structural change, I would prefer to do that as its
own standalone work; adds far too much risk to this patch...
--
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]
