dcapwell commented on code in PR #56:
URL: https://github.com/apache/cassandra-accord/pull/56#discussion_r1295183301
##########
accord-core/src/main/java/accord/messages/WaitOnCommit.java:
##########
@@ -141,13 +147,13 @@ public Void reduce(Void o1, Void o2)
@Override
public void accept(Void result, Throwable failure)
{
- ack();
+ ack(failure);
}
- private void ack()
+ private void ack(@Nullable Throwable fail)
{
if (waitingOnUpdater.decrementAndGet(this) == -1)
- node.reply(replyTo, replyContext, WaitOnCommitOk.INSTANCE);
+ node.reply(replyTo, replyContext, fail != null ?
WaitOnCommitOk.INSTANCE : null, fail);
Review Comment:
this doesn't look right, if we see a failure will we actually decrement
completely? Also wouldn't the exception be lost if it isn't the last event?
##########
accord-core/src/test/java/accord/burn/BurnTestConfigurationService.java:
##########
@@ -66,7 +70,7 @@ public FetchTopologyRequest(long epoch)
public void process(Node on, Node.Id from, ReplyContext replyContext)
{
Topology topology = on.configService().getTopologyForEpoch(epoch);
Review Comment:
this can throw, so prob want a `try/catch`?
##########
accord-core/src/test/java/accord/impl/list/ListAgent.java:
##########
@@ -46,11 +52,16 @@ public ListAgent(long timeout, Consumer<Throwable>
onFailure, Consumer<Runnable>
@Override
public void onRecover(Node node, Result success, Throwable fail)
{
+ if (fail != null)
+ {
+ checkState(success == null, "fail (%s) and success (%s) are both
not null", fail, success);
+ // We don't really process errors for Recover here even though it
is provided in the interface
Review Comment:
we should report back still, else the future can hang forever right?
##########
accord-core/src/test/java/accord/burn/BurnTestConfigurationService.java:
##########
@@ -66,7 +70,7 @@ public FetchTopologyRequest(long epoch)
public void process(Node on, Node.Id from, ReplyContext replyContext)
{
Topology topology = on.configService().getTopologyForEpoch(epoch);
Review Comment:
posted this in slack, prob want to change `Node.recieve` to have
```
scheduler.now(() -> {
try
{
request.process(this, from, replyContext);
}
catch (Throwable t)
{
reply(from, replyContext, null, t);
}
});
```
that way you don't worry about my above comment
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -129,7 +129,9 @@ private void ack(@Nullable Ranges newUnavailable)
// 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)
- reply(this.unavailable, data);
+ {
Review Comment:
style: remove `{}`
##########
accord-core/src/main/java/accord/messages/ReadData.java:
##########
@@ -99,12 +99,12 @@ public synchronized void accept(ReadNack reply, Throwable
failure)
{
if (reply != null)
{
- node.reply(replyTo, replyContext, reply);
+ node.reply(replyTo, replyContext, reply, null);
Review Comment:
```suggestion
node.reply(replyTo, replyContext, reply, failure);
```
you didn't check if failure is non-null, so pass that on to the sink to check
##########
accord-core/src/test/java/accord/impl/list/ListRequest.java:
##########
@@ -110,38 +110,38 @@ public void accept(Result success, Throwable fail)
// TODO (desired, testing): error handling
if (success != null)
{
- node.reply(client, replyContext, (ListResult) success);
+ node.reply(client, replyContext, (ListResult) success, null);
Review Comment:
```suggestion
node.reply(client, replyContext, (ListResult) success, fail);
```
didn't check `fail` yet
##########
accord-core/src/test/java/accord/utils/MessageTask.java:
##########
@@ -88,7 +97,8 @@ public TaskRequest(NodeProcess process, String desc)
@Override
public void process(Node on, Node.Id from, ReplyContext replyContext)
{
- process.process(on, from, success -> on.reply(from, replyContext,
success ? SUCCESS : FAILURE));
+ // TODO (review): This is kind of odd, no error to propagate?
+ process.process(on, from, success -> on.reply(from, replyContext,
success ? SUCCESS : FAILURE, null));
Review Comment:
only happens in `accord.burn.TopologyUpdates#notify` ```long nodeEpoch =
node.epoch();
if (nodeEpoch + 1 < update.epoch())
onDone.accept(false);```
in this single case, an exception at least gives more
detail; which is nice... hit this condition earlier this morning so a human
readable string would be nice
--
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]
