Handle exceptions during event mapping. #113
Conversation
Avoid letting exceptions during event mapping cause multiple events to be lost. Also provide better logging.
|
As requested, a pull against master. |
| doProcess(avroBuffer); | ||
| listener.incomingRequest(event, avroBuffer, avroRecord); | ||
| doProcess(avroBuffer); | ||
| } catch (final RuntimeException e) { |
There was a problem hiding this comment.
The listener call and the doProcess should go outside of the try-catch block, if we're guarding against mapping errors here.
There was a problem hiding this comment.
If you look at the stack trace from #110, you will see it actually propagated up from fromRecord. I guess it wasn't strictly an error in the mapping logic, but rather the generic schema being employed. Either way, I think it is safer to include it.
There was a problem hiding this comment.
But to clarify, listener.incomingRequest & doProcess are both inside the try/catch block.
There was a problem hiding this comment.
Yeah, I would consider the fromRecord to be part of the mapping (going from event to Avro) and doProcess(...) is part of handling the (then valid) avro record for enqueueing. Any error at that point would really be a bug and not a mapping error.
The listener is really just there for testing purposes (there's certain classes in the HTTP implementation that we cannot mock, so we use this path instead). Running Divolte normally shouldn't use it. If there is an exception in the listener, it's clearly a programming error.
There was a problem hiding this comment.
Oh geez. I misread your feedback... I read "inside" where you said "outside". Okay. I'll update the pull.
Neither are parting of the user configurable logic and should really execute without error.
Avoid letting exceptions during event mapping cause multiple events to
be lost. Also provide better logging.