-
Notifications
You must be signed in to change notification settings - Fork 16.4k
Fail the task if the xcom sidecar container does not start for a long time #40909
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fail the task if the xcom sidecar container does not start for a long time #40909
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
8a89262 to
9ee7042
Compare
amoghrajesh
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jhongy1994 good work on your first PR. A few review comments
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just dump the events as well? That would be a better indicator than users manually checking in k8s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add test cases for this change? Like basically call this, sleep for certain seconds and then check if the exception came up
|
I agree with your choice to use |
hussein-awala
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small nit, LGTM
|
@hussein-awala Thank you for your review! |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 5 days if no further activity occurs. Thank you for your contributions. |
|
gentle ping to reviewers @amoghrajesh, @romsharon98, @hussein-awala 😃can we merge this PR? |
5a2e1b5 to
0fee52f
Compare
|
@jhongy1994 can you rebase and resolve conflicts? |
This PR fixes XCOM_sidecar_container not started results in long running DAG
Closes: #38115
The following operators are affected:
Although
startup_timeout_secondsis the timeout for the pod start, there is no parameter to set the timeout for the sidecar container inKubernetesPodOperator. Therefore, I have usedstartup_timeout_secondsfor xcom sidecar container too.Instead of above approach, I have also considered the following alternatives:
attempt > 1instead of using a timeout.If you have any better suggestions, please let me know.
^ 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 newsfragments.