Do not retry after a rejected message #1278
Conversation
awrichar
requested review from
peterbroadhurst,
nguyer and
shorsher
as code owners
Codecov Report
@@ Coverage Diff @@
## main #1278 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 308 308
Lines 20720 20723 +3
=========================================
+ Hits 20720 20723 +3
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
| @@ -535,8 +535,7 @@ func (ag *aggregator) processMessage(ctx context.Context, manifest *core.BatchMa | |||
| np.IncrementNextPin(ctx, ag.namespace) | |||
| } | |||
| state.markMessageDispatched(manifest.ID, msg, msgBaseIndex, newState) | |||
|
|
|||
| return err | |||
| return nil | |||
There was a problem hiding this comment.
Is this a smell that we're not checking the err = ag.checkOnchainConsistency error above?
Or else, what path was it that dropped an error down to here that we did not want to return?
There was a problem hiding this comment.
The contract of this method is that it should only return retryable errors, which should happen on line 522. No new errors can happen between that line and here.
Rejection sometimes returns a reason in the error field, which is generally just logged but shouldn't trigger retry.
We probably could spend another few days cleaning up all these paths though.
There was a problem hiding this comment.
I think the specific path that might return reject with err is via ag.definitions.HandleDefinitionBroadcast.
There was a problem hiding this comment.
I added another commit to log and drop the error along that specific path, which also likely solves the issues I was seeing. But I think it still feels pretty correct to return nil here, at least as the code stands right now. Figuring out how to spell retryable vs. non-retryable errors throughout this whole package has been challenging.
| @@ -607,6 +606,10 @@ func (ag *aggregator) readyForDispatch(ctx context.Context, msg *core.Message, d | |||
| var handlerResult definitions.HandlerResult | |||
| handlerResult, err = ag.definitions.HandleDefinitionBroadcast(ctx, &state.BatchState, msg, data, tx) | |||
| log.L(ctx).Infof("Result of definition broadcast '%s' [%s]: %s", msg.Header.Tag, msg.Header.ID, handlerResult.Action) | |||
| if handlerResult.Action == core.ActionReject { | |||
| log.L(ctx).Infof("Definition broadcast '%s' rejected: %s", msg.Header.ID, err) | |||
| err = nil | |||
There was a problem hiding this comment.
Thanks for adding this 👍
Fixes regression from hyperledger#1251 Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Signed-off-by: Andrew Richardson <andrew.richardson@kaleido.io>
Fixes regression from #1251