Replaced ACK_FAILURE with RESET #511
Conversation
| messageDispatcher.unMuteAckFailure(); | ||
| resetCompleted( completionFuture, success ); | ||
| messageDispatcher.clearCurrentError(); | ||
| if (completionFuture != null) |
There was a problem hiding this comment.
Why this null check is added? Did we changed as old behaviour here?
Why this future could be null and is it safe to not have an else here?
There was a problem hiding this comment.
@nigelsmall review completed. Made two comments. It would also be nice to add a test that asserts valid errors are thrown in an explicit transaction after a failed run. Smth like this for blocking and async API:
Transaction tx = session.beginTransaction();
tx.run( "RETURN invalid" ).consume(); // should throw syntax error
tx.run( "RETURN 1" ).consume(); // should throw some meaningful error
tx.close();| @@ -1157,7 +1157,7 @@ public void shouldFailToCommitWhenRunFailureIsConsumed() | |||
| } | |||
| catch ( ClientException e ) | |||
| { | |||
| assertThat( e.getMessage(), startsWith( "Transaction rolled back" ) ); | |||
| assertThat( e.getMessage(), startsWith( "No current transaction to commit." ) ); | |||
There was a problem hiding this comment.
This message change is quite weird. It exposes an implementation detail that driver resets a connection after an error. Do you think it is okay?
| // try to write ACK_FAILURE before notifying the next response handler | ||
| ackFailureIfNeeded(); | ||
| // write a RESET to "acknowledge" the failure | ||
| queue( new ResetResponseHandler( this, null ) ); |
There was a problem hiding this comment.
Maybe better to create a constructor that only takes an InboundMessageDispatcher and uses null inside ResetResponseHandler. Just to not pass null explicitly as the second parameter.
lutovich
force-pushed
the
1.7-ack-failure-to-reset
branch
from
381ba5f to
63140b4
Compare
Every failure is now "acknowledged" by sending a RESET message. It makes database cleanup the state of the transaction and forget about it. Thus every failure is fatal and transaction can't execute more queries or commit. This commit makes every failure treated as fatal for transactions.
Results of all queries need to be consumed before transaction is committed or rolled back. This is done by `ResultCursorsHolder` which keeps all cursors and queries them for errors. Previously, it only waited for the first failure to arrive and there might've been some more queries receiving results. This commit fixes the problem by making the `ResultCursorsHolder` properly await for all result cursors to be consumed. It does not do blocking await but structures the result future to only be completed when all results have arrived.
lutovich
force-pushed
the
1.7-ack-failure-to-reset
branch
from
63e6b5f to
6707e7a
Compare
No description provided.