Fix EMR Serverless delete operator skipping deletion when deferrable - #73323
Conversation
potiuk
left a comment
There was a problem hiding this comment.
Verified end to end: on main the inherited execute() defers before delete_application is ever reached and the delete operator's own execute_complete then logs "deleted successfully" on the stop trigger's event; the stop_complete_method_name hook routes the resume to delete_stopped_application, so both the plain and force_stop deferrable chains now actually delete.
Checked specifically:
_delete_applicationcarries the whole old tail ofexecute()verbatim; theraise AirflowException("Application deletion failed ...")is a pure relocation, not a new usage.- The only modified existing test (
test_delete_application_deferrable) is strengthened, not loosened. - All three new tests fail on
main(delete_stopped_applicationdoes not exist there), so they are real regression tests. - Non-deferrable path is unchanged and still covered by the pre-existing
test_delete_application_*tests.
Smaller observations
- Two nits inline (a comment that restates the parent, and one
autospec). I'll push fixups for both so this can merge without another round-trip. - The upgrade-path caveat in the description is accurate and unavoidable; no change needed.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Airflow maintainer. The maintainer
approving this PR has read the findings and signed off. If
something feels off, please reply on the PR and a maintainer
will follow up.More on how Apache Airflow handles maintainer review:
contributing-docs/05_pull_requests.rst.
With deferrable=True the parent stop step defers, so the delete call placed after super().execute() was never reached. The task then resumed in the delete operator's execute_complete with the stop trigger's success event, logged that the application was deleted and succeeded, silently leaving the application in place. closes: apache#72123
Review nits: the comment in delete_stopped_application now records why super().execute_complete is used (this class overrides execute_complete for the delete trigger's event), and the cancel_running_jobs patch in the force_stop test uses autospec=True. Generated-by: Claude Opus 5
dd35b49 to
6baa30a
Compare
EmrServerlessDeleteApplicationOperator(deferrable=True)never deleted the application. The inherited stop step defers, so the code aftersuper().execute()that callsDeleteApplicationwas never reached. The task then resumed in the delete operator'sexecute_completewith the stop trigger's success event, logged "deleted successfully" and went green, leaving the application in place (only stopped). The non-deferrable path was unaffected.The stop operator now defers to an overridable
stop_complete_method_name(defaultexecute_complete, so its behaviour is unchanged) in both places it waits onEmrServerlessStopApplicationTrigger, including theforce_stoppath. The delete operator sets it todelete_stopped_application, which validates the stop event, then callsDeleteApplicationand defers onEmrServerlessDeleteApplicationTriggeras before. Deferrable and non-deferrable modes now share the same delete code.The existing
test_delete_application_deferrableonly asserted thatTaskDeferredwas raised, which is why it passed with the bug. It now also checks the trigger, the resume method and that delete isn't called before the stop completes. New tests cover the resume-then-delete step, theforce_stopchain and a failed stop event. All of them fail onmain.Note: task instances already deferred on the stop trigger by an older provider version during an upgrade will still resume in
execute_complete, since the stop and delete trigger events are identical and can't be told apart.Tested locally:
pyteston the amazon provider's EMR operator and trigger tests: 166 passedprekpre-commit stage passes, apart fromcheck-template-fields-valid, which needs Docker and wasn't run locally (no template fields changed);mypyonemr.pypassescloses: #72123
Was generative AI tooling used to co-author this PR?
Generated-by: Claude Code (Opus 5) following the guidelines