[AIRFLOW-4292] Cleanup and improve SLA code by serkef · Pull Request #5517 · apache/airflow

serkef · 2019-07-02T19:58:04Z

Replace unnecessary list allocations with generators
Remove redundant filter
Fix typos
Improve readability and comments

(This continues #5083 which got quite hard to rebase)
Make sure you have checked all steps below.

Jira

My PR addresses the following Airflow Jira issues and references them in the PR title.
- https://issues.apache.org/jira/browse/AIRFLOW-4292

Description

Here are some details about my PR, including screenshots of any UI changes:
This is code cleanup. Some small functional improvements, improve readability by adding comments and fixing typos

Tests

My PR adds the following unit tests OR does not need testing for this extremely good reason:
No new features. This is already covered.

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.
- All the public functions and the classes in the PR contain docstrings that explain what it does
- If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

Passes flake8

* Replace unnecessary list allocators with generators * Remove redundant filter * Fix typos * Improve readability and comments

turbaszek

Could you try to remove this file from pylint TODO list?

turbaszek · 2019-07-10T20:34:09Z

airflow/jobs/scheduler_job.py

+        We are assuming that the scheduler runs often, so we only check for
        tasks that should have succeeded in the past hour.
        """
-        if not any([isinstance(ti.sla, timedelta) for ti in dag.tasks]):


turbaszek · 2019-07-10T20:38:57Z

airflow/jobs/scheduler_job.py

Doing such refactor it would be a good idea to change this sq name to something more pylint compatible. Maybe then we will be able to remove it from TODO list?

Thanks, didn't know about this one. Will change.

hm seems that file has several issues. I would prefer to keep the scope of this ticket on the specific function.

serkef · 2019-07-10T21:16:04Z

airflow/jobs/scheduler_job.py

-                    session.merge(sla)
-            session.commit()
+        if not slas:
+            return


Reversing the control statement for readability. Will comment below on the specific changes.

serkef · 2019-07-10T21:16:38Z

airflow/jobs/scheduler_job.py

+        if not slas:
+            return
+
+        sla_dates = {sla.execution_date for sla in slas}


Changing list to set

serkef · 2019-07-10T21:17:02Z

airflow/jobs/scheduler_job.py

+                session.commit()
+
+        task_list = "\n".join(
+            sla.task_id + ' on ' + sla.execution_date.isoformat()


Changing list to generator

serkef · 2019-07-10T21:17:09Z

airflow/jobs/scheduler_job.py

+            sla.task_id + ' on ' + sla.execution_date.isoformat()
+            for sla in slas)
+        blocking_task_list = "\n".join(
+            ti.task_id + ' on ' + ti.execution_date.isoformat()


Changing list to generator

serkef · 2019-07-10T22:56:19Z

Can one help me with the pylint errors? It complains for accessing unknown property. Issue is class has a method __get_token that is normally translated to _SlackHook__get_token. Naming was not my choice. Should I rename or disable the check?

potiuk · 2019-07-10T23:17:51Z

Neither. I have a change pending approval/merge to CONTRIBUTING.md on how to deal with pylint errors. You should disable the pylint error for that particular line:

https://github.com/PolideaInternal/airflow/blob/ms-travis-ci-tests/CONTRIBUTING.md#pylint-checks-work-in-progress

This reverts commit 80bd78f

codecov-io · 2019-07-10T23:54:36Z

Codecov Report

Merging #5517 into master will decrease coverage by 0.33%.
The diff coverage is 84.78%.

@@            Coverage Diff             @@
##           master    #5517      +/-   ##
==========================================
- Coverage   79.07%   78.74%   -0.34%     
==========================================
  Files         489      489              
  Lines       30744    30727      -17     
==========================================
- Hits        24312    24196     -116     
- Misses       6432     6531      +99

Impacted Files	Coverage Δ
airflow/jobs/scheduler_job.py	`70.43% <84.78%> (+0.15%)`	⬆️
airflow/operators/mysql_operator.py	`0% <0%> (-100%)`	⬇️
airflow/operators/mysql_to_hive.py	`0% <0%> (-100%)`	⬇️
airflow/utils/operator_resources.py	`56.52% <0%> (-30.44%)`	⬇️
airflow/utils/sqlalchemy.py	`74.41% <0%> (-4.66%)`	⬇️
airflow/utils/dag_processing.py	`58.25% <0%> (-2.02%)`	⬇️
airflow/hooks/hive_hooks.py	`75.94% <0%> (-1.78%)`	⬇️
airflow/models/connection.py	`63.88% <0%> (-1.12%)`	⬇️
airflow/utils/helpers.py	`82.58% <0%> (-0.98%)`	⬇️
airflow/hooks/dbapi_hook.py	`87.71% <0%> (-0.88%)`	⬇️
... and 8 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 0fe8cbe...7fa8b2b. Read the comment docs.

stale · 2019-09-03T16:42:25Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

serkef · 2019-09-08T13:33:42Z

Working on it...

stale · 2019-11-26T10:47:17Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Improve and clean SLA code

c99d00f

* Replace unnecessary list allocators with generators * Remove redundant filter * Fix typos * Improve readability and comments

serkef mentioned this pull request Jul 2, 2019

[AIRFLOW-4292] Cleanup and improve SLA code #5083

Closed

6 tasks

turbaszek reviewed Jul 10, 2019

View reviewed changes

serkef commented Jul 10, 2019

View reviewed changes

airflow/jobs/scheduler_job.py

if not slas:

return

sla_dates = {sla.execution_date for sla in slas}

Copy link

Contributor Author

serkef Jul 10, 2019

Choose a reason for hiding this comment

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

Changing list to set

serkef commented Jul 10, 2019

View reviewed changes

serkef added 2 commits July 11, 2019 00:52

Rename var for readability

ba4ff20

Remove this file from pylint todo list

80bd78f

Revert "Remove this file from pylint todo list"

7fa8b2b

This reverts commit 80bd78f

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 3, 2019

stale bot removed the stale Stale PRs per the .github/workflows/stale.yml policy file label Sep 8, 2019

kaxil force-pushed the master branch from 558f9f1 to d719e1f Compare October 2, 2019 15:30

Merge branch 'master' into AIRFLOW-4292_cleanup_improve_SLA

d8f9ff9

stale bot added the stale Stale PRs per the .github/workflows/stale.yml policy file label Nov 26, 2019

stale bot closed this Dec 3, 2019

Conversation

serkef commented Jul 2, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Jira

Description

Tests

Commits

Documentation

Code Quality

Uh oh!

turbaszek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

serkef commented Jul 10, 2019

Uh oh!

potiuk commented Jul 10, 2019

Uh oh!

codecov-io commented Jul 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

stale bot commented Sep 3, 2019

Uh oh!

serkef commented Sep 8, 2019

Uh oh!

stale bot commented Nov 26, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Comments

serkef commented Jul 2, 2019 •

edited

Loading

codecov-io commented Jul 10, 2019 •

edited

Loading