Skip to content

Conversation

@mik-laj
Copy link
Member

@mik-laj mik-laj commented Feb 24, 2020

If we realize that some methods always get one constant value, then their logic can be simplified.


Issue link: AIRFLOW-6907

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*
  • Unit tests coverage for changes (not needed for documentation changes)
  • 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.

* For document-only changes commit message can start with [AIRFLOW-XXXX].


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.

@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Feb 24, 2020
@boring-cyborg boring-cyborg bot added the provider:google Google (including GCP) related issues label Feb 25, 2020
@mik-laj mik-laj requested review from ashb, kaxil, potiuk and turbaszek and removed request for ashb and potiuk February 25, 2020 00:51
@mik-laj mik-laj changed the title [AIRFLOW-6907][WIP] Simplify SchedulerJob [AIRFLOW-6907] Simplify SchedulerJob Feb 25, 2020
@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@d1a3424). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #7527   +/-   ##
=========================================
  Coverage          ?   86.57%           
=========================================
  Files             ?      896           
  Lines             ?    42634           
  Branches          ?        0           
=========================================
  Hits              ?    36910           
  Misses            ?     5724           
  Partials          ?        0
Impacted Files Coverage Δ
airflow/jobs/scheduler_job.py 90.28% <100%> (ø)

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 d1a3424...37c630d. Read the comment docs.

.outerjoin(DM, DM.dag_id == TI.dag_id)
.filter(or_(DM.dag_id == None, # noqa: E711 pylint: disable=singleton-comparison
not_(DM.is_paused)))
.filter(or_(DM.dag_id.is_(None), not_(DM.is_paused)))
Copy link
Member

Choose a reason for hiding this comment

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

@nuclearpinguin This was an example of where we don't need a noqa anymore -- do you know why you did on your pylint fixes to jobs? Is it because we are ignoring this whole file?

Copy link
Member Author

@mik-laj mik-laj Feb 25, 2020

Choose a reason for hiding this comment

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

DM.dag_id == None => disable=singleton-comparison
DM.dag_id.is_(None) is correct syntax.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I'm okay with this. My question is why in #7484 does he have

models.DagRun.state.is_(None)))  # pylint: disable=no-member

but we don't need that here?

Copy link
Member

Choose a reason for hiding this comment

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

(Sorry for not being clear. This comment was not about this PR, but about the other one)

Copy link
Member

@turbaszek turbaszek Feb 25, 2020

Choose a reason for hiding this comment

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

I don't know, but here we do not run pylint over this file, do we? @ashb

Copy link
Member

Choose a reason for hiding this comment

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

It's still in TODO list:
https://github.com/PolideaInternal/airflow/blob/37c630da636d1ef352273e33fbdb417727914386/scripts/ci/pylint_todo.txt#L11

And I suppose that once I rebase onto new master I will have to fix it in more places...

from googleapiclient.errors import HttpError

from airflow import AirflowException
from airflow.exceptions import AirflowException
Copy link
Member

Choose a reason for hiding this comment

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

Unrelated change here?

Copy link
Member

Choose a reason for hiding this comment

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

Other than this, 👍 to the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

This change was already in the master, but the PR preview has not been updated.

@ashb ashb removed the provider:google Google (including GCP) related issues label Feb 25, 2020
@mik-laj mik-laj merged commit 77b5320 into apache:master Feb 25, 2020
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:Scheduler including HA (high availability) scheduler

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants