[AIRFLOW-3561] Improve queries #4368

ffinfo · 2018-12-23T17:44:14Z

Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title. For example, "[AIRFLOW-3561] My Airflow PR"
- https://issues.apache.org/jira/browse/AIRFLOW-3561

Description

This pull request will reduce interaction with the DagBag en improves database queries.

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:

Commits

My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":
1. Subject is separated from body by a blank line
2. Subject is limited to 50 characters (not including Jira issue reference)
3. Subject does not end with a period
4. Subject uses the imperative mood ("add", not "adding")
5. Body wraps at 72 characters
6. Body explains "what" and "why", not "how"

Documentation

In case of new functionality, my PR adds documentation that describes how to use it.
- When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.
- All the public functions and the classes in the PR contain docstrings that explain what it does

Code Quality

Passes flake8

codecov-io · 2018-12-23T18:26:27Z

Codecov Report

Merging #4368 into master will increase coverage by 0.02%.
The diff coverage is 90.9%.

@@            Coverage Diff             @@
##           master    #4368      +/-   ##
==========================================
+ Coverage   78.14%   78.17%   +0.02%     
==========================================
  Files         202      204       +2     
  Lines       16486    16530      +44     
==========================================
+ Hits        12883    12922      +39     
- Misses       3603     3608       +5

Impacted Files	Coverage Δ
airflow/models/__init__.py	`92.81% <100%> (+0.04%)`	⬆️
airflow/www_rbac/views.py	`72.59% <100%> (+0.16%)`	⬆️
airflow/www/views.py	`69.31% <80%> (-0.08%)`	⬇️
airflow/utils/db.py	`32.28% <0%> (-0.26%)`	⬇️
airflow/models/connection.py	`88.6% <0%> (ø)`	⬆️
airflow/models/dagpickle.py	`94.11% <0%> (ø)`
airflow/models/base.py	`85.71% <0%> (ø)`
airflow/jobs.py	`77.41% <0%> (+0.02%)`	⬆️
airflow/bin/cli.py	`64.52% <0%> (+0.04%)`	⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c014324...4db22b1. Read the comment docs.

jmcarp · 2018-12-26T04:04:29Z

airflow/models/__init__.py

+            return self._default_view
+
+    def get_default_view(self):
+        """This is only there for backward compatible jina2 templates"""


I think I'm missing something here. Why can't the templates continue using the default_view property? Also, s/jina2/jinja2/.

This was to do with https://issues.apache.org/jira/browse/AIRFLOW-3562.
Currently DAG is still used in other templates, can't change all those at once. Once all views has switched to DagModel this method can be removed again.

ps: fixing typo ;)

jmcarp · 2018-12-26T04:19:50Z

airflow/models/__init__.py

@@ -4201,6 +4222,8 @@ def sync_to_db(self, owner=None, sync_time=None, session=None):
        orm_dag.owners = owner
        orm_dag.is_active = True
        orm_dag.last_scheduler_run = sync_time
+        orm_dag.default_view = self._default_view


Can we test this in tests.models:DagTest.test_sync_to_db?

Good one, added a test

jmcarp · 2018-12-26T04:23:00Z

airflow/models/__init__.py

@@ -3249,6 +3259,17 @@ def __exit__(self, _type, _value, _tb):

    # /Context Manager ----------------------------------------------

+    @property
+    def default_view(self):


Can we test this property?

Good one, added a test

This behavior how differs from the get_default_view method. Maybe we can remove the get_default_view method completely: https://stackoverflow.com/questions/49134141/how-do-i-reference-an-object-property-in-a-method-call-in-jinja2

This would reduce the number of methods on the DAG object. Besides that, I believe that dag.default_view should return the same as dag.get_default_view() since the names are so similar.

@Fokko Agree there. I did now remove all usage of dag.default_view and switch it to dag.get_default_view()

@Fokko: Can't use just default_view because in DagModel this can be None and get_default_view has a fall back to the config when not set.

Awesome, thanks!

jmcarp · 2018-12-26T04:24:48Z

Until #4339 is merged, we have to make duplicate changes to airflow/www/views and airflow/www_rbac/views.

Fokko

Small suggestions, apart from that it looks great!

Fokko · 2018-12-26T09:48:37Z

airflow/models/__init__.py

@@ -3249,6 +3259,17 @@ def __exit__(self, _type, _value, _tb):

    # /Context Manager ----------------------------------------------

+    @property
+    def default_view(self):


This behavior how differs from the get_default_view method. Maybe we can remove the get_default_view method completely: https://stackoverflow.com/questions/49134141/how-do-i-reference-an-object-property-in-a-method-call-in-jinja2

This would reduce the number of methods on the DAG object. Besides that, I believe that dag.default_view should return the same as dag.get_default_view() since the names are so similar.

Fokko · 2018-12-26T09:54:52Z

airflow/www/views.py

-        qry = (
-            session.query(ds.dag_id, ds.state, ds.count)
-        )
+        states = session.query(dr.dag_id, dr.state, sqla.func.count(dr.state)).group_by(dr.dag_id, dr.state)

        data = {}


Maybe give this a bit more meaningful name? Something like dag_state_stats.

Fokko · 2018-12-26T09:57:46Z

Two related CI issues:


======================================================================
3) ERROR: test_sync_to_db (tests.DagTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    .tox/py27-backend_mysql-env_docker/lib/python2.7/site-packages/mock/mock.py line 1305 in patched
      return func(*args, **keywargs)
    tests/models.py line 749 in test_sync_to_db
      self.assertIsNone(orm_dag._default_view)
   AttributeError: 'DagModel' object has no attribute '_default_view'
   -------------------- >> begin captured stdout << ---------------------
   [2018-12-26 08:08:11,955] {__init__.py:4219} INFO - Creating ORM DAG for dag
   [2018-12-26 08:08:11,963] {__init__.py:4219} INFO - Creating ORM DAG for dag.subtask
   
   --------------------- >> end captured stdout << ----------------------
   -------------------- >> begin captured logging << --------------------
   airflow.models.DAG: INFO: Creating ORM DAG for dag
   airflow.models.DAG: INFO: Creating ORM DAG for dag.subtask
   --------------------- >> end captured logging << ---------------------
======================================================================
4) ERROR: test_sync_to_db_default_view (tests.DagTest)
----------------------------------------------------------------------
   Traceback (most recent call last):
    .tox/py27-backend_mysql-env_docker/lib/python2.7/site-packages/mock/mock.py line 1305 in patched
      return func(*args, **keywargs)
    tests/models.py line 781 in test_sync_to_db_default_view
      self.assertIsNotNone(orm_dag._default_view)
   AttributeError: 'DagModel' object has no attribute '_default_view'

ffinfo · 2018-12-26T11:13:18Z

Rbac is now also fixed

* improve queries * Adding field to the database * Set length of field * remove dagbag use in xcom call * Fixing typo * Adding test * Remove default_view * fixing test * rename var * Fixing rbac dag_stats * Fixing rbac task_stats * Fixing rbac code * Fixing rbac xcom * Fixing template * Fixing default view call * Added timezone to DagModel * Fixing timezone

ashb · 2019-01-11T19:13:10Z

airflow/models/__init__.py


    def __repr__(self):
        return "<DAG: {self.dag_id}>".format(self=self)

+    @property
+    def timezone(self):
+        return settings.TIMEZONE


Wait up - this looks incorrect. Why was this removed?

The timezone of a DAG was previously defined as the timezone of the start_date. Is that still the case and if we do self.timezone = tz then this property accessor won't be used anymore?

I don't mean removed, I mean added.

Oh this is on the DagModel, not the DAG. Right.

In which case my question is: when/where is this used?

This is indeed a bit tricky. I have added this because in the html templates this variable is used. Some templates now takes a DagModel instead of a DAG. In the end I would like this to be in the complete webserver

* improve queries * Adding field to the database * Set length of field * remove dagbag use in xcom call * Fixing typo * Adding test * Remove default_view * fixing test * rename var * Fixing rbac dag_stats * Fixing rbac task_stats * Fixing rbac code * Fixing rbac xcom * Fixing template * Fixing default view call * Added timezone to DagModel * Fixing timezone

ashb · 2019-03-05T21:31:58Z

We reverted this from master? Did it ever make it back in in any form?

ffinfo · 2019-03-07T10:57:19Z

@ashb Seems to be still in there:
https://github.com/apache/airflow/blame/master/airflow/www/views.py#L299

Because of the switch to the rbac interface it's was moved I think

* improve queries * Adding field to the database * Set length of field * remove dagbag use in xcom call * Fixing typo * Adding test * Remove default_view * fixing test * rename var * Fixing rbac dag_stats * Fixing rbac task_stats * Fixing rbac code * Fixing rbac xcom * Fixing template * Fixing default view call * Added timezone to DagModel * Fixing timezone

#5111) We used to key on `safe_dag_id` but in that got changed in #4368 to use a more efficient query, which broke for non-sub-DAGs that contain a `.` in their id. Arguably this escaping is a front-end concern anyway, so handling the escaping in the front end makes sense anyway.

apache#5111) We used to key on `safe_dag_id` but in that got changed in apache#4368 to use a more efficient query, which broke for non-sub-DAGs that contain a `.` in their id. Arguably this escaping is a front-end concern anyway, so handling the escaping in the front end makes sense anyway. (cherry picked from commit c696dd4)

apache#5111) We used to key on `safe_dag_id` but in that got changed in apache#4368 to use a more efficient query, which broke for non-sub-DAGs that contain a `.` in their id. Arguably this escaping is a front-end concern anyway, so handling the escaping in the front end makes sense anyway.

apache#5111) We used to key on `safe_dag_id` but in that got changed in apache#4368 to use a more efficient query, which broke for non-sub-DAGs that contain a `.` in their id. Arguably this escaping is a front-end concern anyway, so handling the escaping in the front end makes sense anyway. (cherry picked from commit c696dd4)

apache#5111) We used to key on `safe_dag_id` but in that got changed in apache#4368 to use a more efficient query, which broke for non-sub-DAGs that contain a `.` in their id. Arguably this escaping is a front-end concern anyway, so handling the escaping in the front end makes sense anyway.

* improve queries * Adding field to the database * Set length of field * remove dagbag use in xcom call * Fixing typo * Adding test * Remove default_view * fixing test * rename var * Fixing rbac dag_stats * Fixing rbac task_stats * Fixing rbac code * Fixing rbac xcom * Fixing template * Fixing default view call * Added timezone to DagModel * Fixing timezone

apache#5111) We used to key on `safe_dag_id` but in that got changed in apache#4368 to use a more efficient query, which broke for non-sub-DAGs that contain a `.` in their id. Arguably this escaping is a front-end concern anyway, so handling the escaping in the front end makes sense anyway.

improve queries

bed94ae

ffinfo added 3 commits December 23, 2018 22:28

Adding field to the database

83e9b7f

Set length of field

8ee3210

remove dagbag use in xcom call

acf194a

jmcarp reviewed Dec 26, 2018

View reviewed changes

ffinfo added 2 commits December 26, 2018 09:00

Fixing typo

0d6ebc3

Adding test

fb1539d

Fokko requested changes Dec 26, 2018

View reviewed changes

Fokko changed the title ~~AIRFLOW-3561 - improve queries~~ [AIRFLOW-3561] Improve queries Dec 26, 2018

ffinfo added 7 commits December 26, 2018 11:39

Remove default_view

48ba0c2

fixing test

77a73c7

rename var

3c04289

Fixing rbac dag_stats

5189257

Fixing rbac task_stats

0b2d538

Fixing rbac code

c14bf93

Fixing rbac xcom

3579995

ffinfo added 4 commits December 26, 2018 12:25

Fixing template

f3bec4f

Merge remote-tracking branch 'origin/master' into improve_queries

bf4e6dd

Fixing default view call

4cffec2

Added timezone to DagModel

bcacbf5

Fokko approved these changes Dec 26, 2018

View reviewed changes

Fixing timezone

4db22b1

Fokko merged commit 0cc078a into apache:master Dec 27, 2018

XD-DENG mentioned this pull request Dec 28, 2018

[AIRFLOW-3587] Remove unnecessary condition checks in dag_stats & task_stats #4395

Merged

2 tasks

ashb reviewed Jan 11, 2019

View reviewed changes

Fokko mentioned this pull request Jan 11, 2019

[AIRFLOW-3678] Fixes 'airflow resetdb' for initialized Postgres DB #4488

Closed

6 tasks

kkmueller mentioned this pull request Aug 23, 2019

[AIRFLOW-4294] Fix missing dag & task runs in UI dag_id contains a dot (#5111) postmates/airflow#9

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[AIRFLOW-3561] Improve queries #4368

[AIRFLOW-3561] Improve queries #4368

ffinfo commented Dec 23, 2018 •

edited

Loading

codecov-io commented Dec 23, 2018 •

edited

Loading

jmcarp Dec 26, 2018

ffinfo Dec 26, 2018

ffinfo Dec 26, 2018

jmcarp Dec 26, 2018

ffinfo Dec 26, 2018

jmcarp Dec 26, 2018

ffinfo Dec 26, 2018

Fokko Dec 26, 2018

ffinfo Dec 26, 2018

ffinfo Dec 26, 2018

Fokko Dec 26, 2018

jmcarp commented Dec 26, 2018

Fokko left a comment

Fokko Dec 26, 2018

Fokko Dec 26, 2018

ffinfo Dec 26, 2018

Fokko commented Dec 26, 2018

ffinfo commented Dec 26, 2018

ashb Jan 11, 2019

ashb Jan 11, 2019

ashb Jan 11, 2019

ffinfo Jan 12, 2019

ashb commented Mar 5, 2019

ffinfo commented Mar 7, 2019

[AIRFLOW-3561] Improve queries #4368

[AIRFLOW-3561] Improve queries #4368

Conversation

ffinfo commented Dec 23, 2018 • edited Loading

Jira

Description

Tests

Commits

Documentation

Code Quality

codecov-io commented Dec 23, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmcarp commented Dec 26, 2018

Fokko left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fokko commented Dec 26, 2018

ffinfo commented Dec 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashb commented Mar 5, 2019

ffinfo commented Mar 7, 2019

ffinfo commented Dec 23, 2018 •

edited

Loading

codecov-io commented Dec 23, 2018 •

edited

Loading