Fix: Only defer EmrCreateJobFlowOperator when wait_policy is set #56077
Conversation
laksh-krishna-sharma
requested review from
eladkal and
o-nikolas
as code owners
boring-cyborg
bot
added
area:providers
provider:amazon
|
@eladkal and @o-nikolas Please review the changes, and let me know if further adjustments are needed. |
|
If you want this PR to be merged, please stop rebasing it. We need the CI to finish before merging it. And every-time, you update this PR, all tests are restarting. If you actually make some changes to this PR, then ignore this message :) |
|
Static checks are failing, can you please fix them? You can find more information about static checks here: https://github.com/apache/airflow/blob/main/contributing-docs/08_static_code_checks.rst |
|
sure i will check and fix failing test |
o-nikolas
left a comment
o-nikolas
left a comment
There was a problem hiding this comment.
The code changes look fine (other than the static check failures).
But I don't love the non-standard wait configuration that this operators uses. Emr is the only one that uses wait_policy. This fix is okay for now, but I'd be in favour of dropping wait_policy altogether (with a deprecation of course), unless someone knows of a good reason it was added in the first place?
|
I agree with Niko, somehow this operator does not follow the standard in Amazon provider package. I created an issue to track that: #56153. In the meantime, merging this PR. |
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
|
Thank you @vincbeck @o-nikolas for reviewing and merging this PR! This was my first contribution to Apache Airflow, and I really appreciate the guidance and feedback from the maintainers and community. Excited to keep learning and contributing more! |
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
…he#56077) * fixing emr operator deferral logic * fixed failing test ruff lint
3 participants
Summary of Changes
This PR fixes the behavior of
EmrCreateJobFlowOperatorwhen global deferrable mode is enabled (AIRFLOW__OPERATORS__DEFAULT_DEFERRABLE=true).Previously, the operator would defer unconditionally if
deferrable=True, even when no wait policy was requested. This caused submit-only jobs (e.g.,wait_policy=None) to unexpectedly defer. With this fix, the operator now only defers when a wait policy is explicitly provided.Closes #40966
Changes Made
Updated
EmrCreateJobFlowOperatorlogic:wait_policyis set anddeferrable=True.wait_policyis set anddeferrable=False.wait_policy=None(submit-only mode).Adjusted tests to cover all scenarios:
TaskDeferred).get_waiter().wait.