Prevent Triggerer from crashing when a trigger event isn't serializable #60152
Conversation
…mplementedError as a trigger event is not serializable, then retry without that event and cancel associated trigger
|
Looks good and seems it would be great to have it in 3.1.6 - but I would love someone with more triggerer expertise reviews it. |
potiuk
added
the
backport-to-v3-1-test
Agreed. Some small explanation of what the code does: Now before the completed events are being send through the comms supervisor, it will first try to encode each event individually, if that succeeds, then the event is considered valid, otherwise it will be considered as failed and thus the trigger will also be considered as a failed trigger due to the fact that it returned a TriggerEvent which contains an non-serializable payload. The drawback is duplicate encoding, once per event and then on the whole message at the end which was originally also the case, but that's the only way I found to be able to detect which event could be the culprit. |
uranusjr
left a comment
uranusjr
left a comment
There was a problem hiding this comment.
I’m surprised static checkers didn’t pick this up.
| await trigger_runner.sync_state_to_supervisor(finished_ids=[]) | ||
|
|
||
| assert trigger_runner.comms_decoder.asend.call_count == 1 | ||
| assert len(trigger_runner.comms_decoder.asend.call_args_list[0].args[0].events) == 2 |
There was a problem hiding this comment.
Only 2 events? Shouldn't this be 3 as we have two success and one failure to report?
ashb
left a comment
ashb
left a comment
There was a problem hiding this comment.
I wonder if its possible to use https://jcristharif.com/msgspec/api.html#raw somehow to avoid the "encode it twice" behaviour. Not sure, but would be nice if we could avoid that.
Co-authored-by: Ash Berlin-Taylor <ash_github@firemirror.com>
Another option would be to try to encode the whole msg, and if it fails then do the validation, that way if it succeeds there is only one encoding and individual encoding would only be done if the full one fails, which is exceptional as it won't happen that often. |
5 participants
This PR is related to the issue #60120 in which the trigger process crashes if a returned trigger event isn't serializable. This PR will catch the NotImpelmentedError and detect which event caused the crash and cancel the trigger associated to that event that caused the error.
^ Add meaningful description above
Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named
{pr_number}.significant.rstor{issue_number}.significant.rst, in airflow-core/newsfragments.