Skip to content

Conversation

@turbaszek
Copy link
Member

@turbaszek turbaszek commented Jun 18, 2020

Currently to access dag in Webserver we use global variable dagbag from airflow.www.views. This global object is also necessary for new Connexion API (/tasks endpoint for example).

However, it would be better to avoid any coupling between views code and API code. This PR proposes to replace this global object with an attribute of Flask app that can be accessed using flask.current_app as follows:

from flask import current_app

@expose('/my_endpoint')
def my_endpoint():
    dag_bag: DagBag = current_app.dag_bag
    ...

Make sure to mark the boxes below before creating PR: [x]

  • Description above provides context of the change
  • Unit tests coverage for changes (not needed for documentation changes)
  • Target Github ISSUE in description if exists
  • Commits follow "How to write a good git commit message"
  • Relevant documentation is updated including usage instructions.
  • I will engage committers as explained in Contribution Workflow Example.

In case of fundamental code change, 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 UPDATING.md.
Read the Pull Request Guidelines for more information.

@turbaszek turbaszek requested a review from mik-laj June 18, 2020 14:13
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Jun 18, 2020
@turbaszek
Copy link
Member Author

@mik-laj and @ephraimbuddy you may want to take a look

@turbaszek turbaszek mentioned this pull request Jun 18, 2020
6 tasks
@ephraimbuddy ephraimbuddy mentioned this pull request Jun 18, 2020
6 tasks
@ephraimbuddy
Copy link
Contributor

There's one test that is failing. pytest tests/www/test_views.py -k test_failed_success. From the name, it seems to be the expected behaviour?

@mik-laj
Copy link
Member

mik-laj commented Jun 19, 2020

I was afraid that this change could affect the time it took to run the tests. However, I checked it and saw no degradation.
Before:

==================================================================== 1 failed, 169 passed, 6 skipped, 134 warnings in 370.20s (0:06:10) ====================================================================

After:

==================================================================== 1 failed, 169 passed, 6 skipped, 134 warnings in 366.13s (0:06:06) ====================================================================

I also did manual tests and didn't notice any problems.

@turbaszek turbaszek merged commit 50318f8 into apache:master Jun 19, 2020
@turbaszek turbaszek deleted the use-current-app-dagbag branch June 19, 2020 08:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants