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] raise exception with main notebook error in DatabricksRunNowDeferrableOperator #39110

Merged
merged 12 commits into from
May 1, 2024

Conversation

gaurav7261
Copy link
Contributor

@gaurav7261 gaurav7261 commented Apr 18, 2024

  1. Implement async method for getting job run output
  2. logic for getting main notebook exception via async api calling and raising the exception

^ 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.

@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch from 541bedc to bd6e798 Compare April 18, 2024 11:56
@gaurav7261
Copy link
Contributor Author

@Lee-W changes are done, please review and let me know

@Lee-W Lee-W self-requested a review April 19, 2024 03:23
@gaurav7261
Copy link
Contributor Author

Hi @Lee-W , is there any issue with PR

@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch from 1f6af3b to 747a1f9 Compare April 21, 2024 21:02
@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch from 747a1f9 to ab91c88 Compare April 21, 2024 21:34
@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch 5 times, most recently from 1a5445f to d8c2ba1 Compare April 22, 2024 18:02
@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch from d8c2ba1 to f406db4 Compare April 22, 2024 18:18
@gaurav7261 gaurav7261 requested a review from Lee-W April 22, 2024 18:34
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.

LGTM

@Lee-W Lee-W requested a review from pankajkoti April 23, 2024 14:00
@gaurav7261
Copy link
Contributor Author

Hi @pankajkoti , can you please review

@Lee-W
Copy link
Member

Lee-W commented Apr 25, 2024

I think we're good to merge this one. Just want to see whether @pankajkoti have some time to double check if I missed anything.

I'm going to merge this later today. Please let me know if anyone want to take a deeper look. 🙂

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.

Can you please update the PR title and description by taking hints from https://cbea.ms/git-commit/ or something similar to mention what and why we're doing.

Also the would be nice in general for PR title to be in imperative mood like mentioned in https://cbea.ms/git-commit/#imperative

airflow/providers/databricks/triggers/databricks.py Outdated Show resolved Hide resolved
airflow/providers/databricks/triggers/databricks.py Outdated Show resolved Hide resolved
@gaurav7261 gaurav7261 changed the title [FEAT] added notebook error in databricks deferrable handler [FEAT] raise exception with main notebook error in DatabricksRunNowDeferrableOperator Apr 25, 2024
@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch from 7158a46 to 1c0709b Compare April 25, 2024 17:02
@Lee-W Lee-W requested review from Lee-W and removed request for Lee-W April 26, 2024 00:39
@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch from 1c0709b to 805544c Compare April 26, 2024 20:25
@gaurav7261 gaurav7261 requested a review from Lee-W April 26, 2024 20:27
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.

Left one nitpick. But I'm good with this PR now 👍 @pankajkoti could you please take another look? Thanks!

@pankajkoti
Copy link
Member

I added my concern here #39110 (comment). But leave the decision to you @Lee-W :)

@gaurav7261 gaurav7261 force-pushed the feat/databricks_deferrable_exception branch from f3f7e8b to 3f6d898 Compare April 30, 2024 11:10
@gaurav7261 gaurav7261 requested a review from Lee-W April 30, 2024 11:35
@gaurav7261
Copy link
Contributor Author

gaurav7261 commented Apr 30, 2024

@Lee-W @pankajkoti please review, deferrable changes done as per review

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

@Lee-W Lee-W merged commit 42dbcca into apache:main May 1, 2024
40 checks passed
@gaurav7261
Copy link
Contributor Author

Hi @Lee-W @pankajkoti , raised PR for runnow operator separately as discussed, please review #39354

@Lee-W
Copy link
Member

Lee-W commented May 2, 2024

Hi @Lee-W @pankajkoti , raised PR for runnow operator separately as discussed, please review #39354

Thanks for your prompt implementation! Will look into it today

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.

3 participants