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

Add log lookup exception for empty op subtypes #35536

Merged
merged 2 commits into from
Jan 12, 2024

Conversation

vchiapaikeo
Copy link
Contributor

closes: #34228

image

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

@eladkal
Copy link
Contributor

eladkal commented Jan 10, 2024

@vchiapaikeo are you still working on this PR?

@vchiapaikeo vchiapaikeo marked this pull request as ready for review January 10, 2024 12:45
@vchiapaikeo
Copy link
Contributor Author

Whoops, sorry about that. Rebased and ready for review here. I retested and it still works as expected -

image

Comment on lines -81 to -82
import httpx

Copy link
Contributor

Choose a reason for hiding this comment

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

Why was this import moved to top of the file? is it due to pre commit change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I did this because I need this module on line 506 (httpx.UnsupportedProtocol). I looked at the blame and there didn't seem to be any reason for this to be function scoped so moving it to module scoped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it looks like Ash moves this out of module scope to function for perf optimization here: 1a8a897#diff-e7f34f73940eb52d92bb991abedc1c963431c5373c12dff739c8fb7d03e93d3aL24

Going to move it back to function scope in a follow up PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR: #36753

@eladkal eladkal merged commit ddcaef4 into apache:main Jan 12, 2024
53 checks passed
ephraimbuddy pushed a commit that referenced this pull request Jan 15, 2024
* Add log lookup exception for empty op subtypes

* Use exception catching approach instead to preserve tests

(cherry picked from commit ddcaef4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:logging type:bug-fix Changelog: Bug Fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DAGs log view of EmptyOperator shows confusing error Request URL missing protocol.
2 participants