Skip to content
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

[FEAT] get all failed tasks errors in exception #39354

Merged

Conversation

gaurav7261
Copy link
Contributor

  1. logic for getting main notebook exception via get_run_output api calling and raising the exception with all failed tasks errors

^ 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.rst or {issue_number}.significant.rst, in newsfragments.

Copy link
Contributor

@dirrao dirrao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about the test case when multiple parallel tasks failed? is that a valid scenario?

@gaurav7261
Copy link
Contributor Author

gaurav7261 commented May 2, 2024

How about the test case when multiple parallel tasks failed? is that a valid scenario?

yes, The current logic is iterating through all failed tasks, hitting get_run_output api and appending the errors in the failed_tasks list. Do you want me to write one more test case with multiple failed tasks @dirrao ?

Copy link
Member

@pankajkoti pankajkoti left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. And yes, would be nice to have that additional test case as mentioned earlier from @dirrao

Copy link
Member

@Lee-W Lee-W left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. but yep, would be great if we could have tests cover this change 🙂

@gaurav7261 gaurav7261 force-pushed the feat/get_all_error_databrick_runnow_operator branch 2 times, most recently from 1ac4763 to b6eaf9d Compare May 2, 2024 21:37
@gaurav7261 gaurav7261 force-pushed the feat/get_all_error_databrick_runnow_operator branch from b6eaf9d to 2c8b929 Compare May 2, 2024 21:52
@gaurav7261 gaurav7261 requested a review from dirrao May 2, 2024 22:11
@Lee-W
Copy link
Member

Lee-W commented May 3, 2024

As it's approved by many, I think we're good to merge this one

@Lee-W Lee-W merged commit 2d103e1 into apache:main May 3, 2024
40 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants