You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/02/21 08:23:36 UTC

[GitHub] [airflow] mik-laj opened a new pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857, AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG

mik-laj opened a new pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857,AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481
 
 
   
   
   ---
   Issue link: WILL BE INSERTED BY [boring-cyborg](https://github.com/kaxil/boring-cyborg)
   
   Make sure to mark the boxes below before creating PR: [x]
   
   - [ ] Description above provides context of the change
   - [ ] Commit message/PR title starts with `[AIRFLOW-NNNN]`. AIRFLOW-NNNN = JIRA ID<sup>*</sup>
   - [ ] Unit tests coverage for changes (not needed for documentation changes)
   - [ ] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
   - [ ] Relevant documentation is updated including usage instructions.
   - [ ] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
   
   <sup>*</sup> For document-only changes commit message can start with `[AIRFLOW-XXXX]`.
   
   ---
   In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
   In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
   In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
   Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] codecov-io commented on issue #7481: [AIRFLOW-6862][depends on AIRFLOW-6857, AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #7481: [AIRFLOW-6862][depends on AIRFLOW-6857,AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#issuecomment-589562903
 
 
   # [Codecov](https://codecov.io/gh/apache/airflow/pull/7481?src=pr&el=h1) Report
   > Merging [#7481](https://codecov.io/gh/apache/airflow/pull/7481?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/1a9a9f7618f1c22e3e9a6ef4ec73b717c7760c7d?src=pr&el=desc) will **decrease** coverage by `0.27%`.
   > The diff coverage is `97.95%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/airflow/pull/7481/graphs/tree.svg?width=650&token=WdLKlKHOAU&height=150&src=pr)](https://codecov.io/gh/apache/airflow/pull/7481?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff            @@
   ##           master   #7481      +/-   ##
   =========================================
   - Coverage   86.68%   86.4%   -0.28%     
   =========================================
     Files         882     882              
     Lines       41526   41607      +81     
   =========================================
   - Hits        35997   35951      -46     
   - Misses       5529    5656     +127
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/airflow/pull/7481?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [airflow/utils/dag\_processing.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9kYWdfcHJvY2Vzc2luZy5weQ==) | `88.22% <ø> (+0.1%)` | :arrow_up: |
   | [airflow/dag/base\_dag.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9kYWcvYmFzZV9kYWcucHk=) | `69.56% <ø> (+1.56%)` | :arrow_up: |
   | [airflow/jobs/scheduler\_job.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9qb2JzL3NjaGVkdWxlcl9qb2IucHk=) | `89.66% <100%> (ø)` | :arrow_up: |
   | [airflow/utils/db.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy91dGlscy9kYi5weQ==) | `98.29% <100%> (-0.02%)` | :arrow_down: |
   | [airflow/models/dag.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9tb2RlbHMvZGFnLnB5) | `91.06% <97.56%> (+0.16%)` | :arrow_up: |
   | [airflow/kubernetes/volume\_mount.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZV9tb3VudC5weQ==) | `44.44% <0%> (-55.56%)` | :arrow_down: |
   | [airflow/kubernetes/volume.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3ZvbHVtZS5weQ==) | `52.94% <0%> (-47.06%)` | :arrow_down: |
   | [airflow/kubernetes/pod\_launcher.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3BvZF9sYXVuY2hlci5weQ==) | `47.18% <0%> (-45.08%)` | :arrow_down: |
   | [...viders/cncf/kubernetes/operators/kubernetes\_pod.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9wcm92aWRlcnMvY25jZi9rdWJlcm5ldGVzL29wZXJhdG9ycy9rdWJlcm5ldGVzX3BvZC5weQ==) | `69.38% <0%> (-25.52%)` | :arrow_down: |
   | [airflow/kubernetes/refresh\_config.py](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree#diff-YWlyZmxvdy9rdWJlcm5ldGVzL3JlZnJlc2hfY29uZmlnLnB5) | `50.98% <0%> (-23.53%)` | :arrow_down: |
   | ... and [5 more](https://codecov.io/gh/apache/airflow/pull/7481/diff?src=pr&el=tree-more) | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/7481?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/airflow/pull/7481?src=pr&el=footer). Last update [1a9a9f7...c5d237b](https://codecov.io/gh/apache/airflow/pull/7481?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r382544484
 
 

 ##########
 File path: airflow/jobs/scheduler_job.py
 ##########
 @@ -686,10 +686,6 @@ def _process_dags(self, dagbag, dags, tis_out):
         """
         check_slas = conf.getboolean('core', 'CHECK_SLAS', fallback=True)
         for dag in dags:
-            dag = dagbag.get_dag(dag.dag_id)
 
 Review comment:
   This will never happen because we have loaded DAG file one level up.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj merged pull request #7481: [AIRFLOW-6862] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
mik-laj merged pull request #7481: [AIRFLOW-6862] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481
 
 
   

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][DONT-MERGE] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][DONT-MERGE] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r384934871
 
 

 ##########
 File path: airflow/jobs/scheduler_job.py
 ##########
 @@ -668,16 +668,14 @@ def _process_task_instances(self, dag, task_instances_list, session=None):
                     self.log.debug('Queuing task: %s', ti)
                     task_instances_list.append(ti.key)
 
-    def _process_dags(self, dagbag, dags, tis_out):
+    def _process_dags(self, dags, tis_out):
 
 Review comment:
   I prefer not to add type annotations for core classes. We have a problem with cyclical imports.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] Fokko commented on a change in pull request #7481: [AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #7481: [AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r383705449
 
 

 ##########
 File path: tests/jobs/test_scheduler_job.py
 ##########
 @@ -1010,7 +1010,7 @@ def test_process_dags_not_create_dagrun_for_subdags(self):
         scheduler._process_task_instances = mock.MagicMock()
         scheduler.manage_slas = mock.MagicMock()
 
-        scheduler._process_dags(self.dagbag, [dag] + dag.subdags, [])
+        scheduler._process_dags([dag] + dag.subdags, [])
 
 Review comment:
   Maybe make the second argument optional? Instead of passing an empty iterable.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r382544484
 
 

 ##########
 File path: airflow/jobs/scheduler_job.py
 ##########
 @@ -686,10 +686,6 @@ def _process_dags(self, dagbag, dags, tis_out):
         """
         check_slas = conf.getboolean('core', 'CHECK_SLAS', fallback=True)
         for dag in dags:
-            dag = dagbag.get_dag(dag.dag_id)
 
 Review comment:
   This will never happen because we have loaded this file one level up.

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r382559756
 
 

 ##########
 File path: airflow/jobs/scheduler_job.py
 ##########
 @@ -686,10 +686,6 @@ def _process_dags(self, dagbag, dags, tis_out):
         """
         check_slas = conf.getboolean('core', 'CHECK_SLAS', fallback=True)
         for dag in dags:
-            dag = dagbag.get_dag(dag.dag_id)
 
 Review comment:
   No. I removed in fixup, but I forgot to push changes. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on issue #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
mik-laj commented on issue #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#issuecomment-589648006
 
 
   I found one more optimization and, by the way, changed the base for this PR.
   Before:
   Query count:  1197
   Average took 4961.877 ms
   After:
   Query count:  799
   Average took 3452.471 ms

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] kaxil commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #7481: [AIRFLOW-6862][depends on AIRFLOW-6857][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r382557401
 
 

 ##########
 File path: airflow/jobs/scheduler_job.py
 ##########
 @@ -686,10 +686,6 @@ def _process_dags(self, dagbag, dags, tis_out):
         """
         check_slas = conf.getboolean('core', 'CHECK_SLAS', fallback=True)
         for dag in dags:
-            dag = dagbag.get_dag(dag.dag_id)
 
 Review comment:
   If that is the case, do we even need dagbag in the method signature?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #7481: [AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r383813761
 
 

 ##########
 File path: tests/jobs/test_scheduler_job.py
 ##########
 @@ -1010,7 +1010,7 @@ def test_process_dags_not_create_dagrun_for_subdags(self):
         scheduler._process_task_instances = mock.MagicMock()
         scheduler.manage_slas = mock.MagicMock()
 
-        scheduler._process_dags(self.dagbag, [dag] + dag.subdags, [])
+        scheduler._process_dags([dag] + dag.subdags, [])
 
 Review comment:
   In another PR I deletes the tis_out parameter completely.  This parameter is also required because this method returns values using this list. Unfortunately, we do not have an assertion in the tests. In real code is always required. 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services

[GitHub] [airflow] Fokko commented on a change in pull request #7481: [AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG

Posted by GitBox <gi...@apache.org>.
Fokko commented on a change in pull request #7481: [AIRFLOW-6862][WIP] Do not check the freshness of fresh DAG
URL: https://github.com/apache/airflow/pull/7481#discussion_r383705120
 
 

 ##########
 File path: airflow/jobs/scheduler_job.py
 ##########
 @@ -668,16 +668,14 @@ def _process_task_instances(self, dag, task_instances_list, session=None):
                     self.log.debug('Queuing task: %s', ti)
                     task_instances_list.append(ti.key)
 
-    def _process_dags(self, dagbag, dags, tis_out):
+    def _process_dags(self, dags, tis_out):
 
 Review comment:
   While at it, can we add type annotations?

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


With regards,
Apache Git Services