Skip to content

Move DAG._schedule_interval logic out of DAG.__init__#8225

Merged
kaxil merged 3 commits intoapache:masterfrom
kaxil:schedule_interal_norm
Apr 9, 2020
Merged

Move DAG._schedule_interval logic out of DAG.__init__#8225
kaxil merged 3 commits intoapache:masterfrom
kaxil:schedule_interal_norm

Conversation

@kaxil
Copy link
Copy Markdown
Member

@kaxil kaxil commented Apr 9, 2020

closes #8166

  • Replace usage of DAG._schedule_interval with DAG.normalized_schedule_interval. This will even allow users to tests that they set correct Cron Preset in unit tests (instead of using an internal property before)
  • Added tests for Schedule Interval & Normalized Schedule Interval
  • Also solves the issue mentioned in Task Duration Bug: Doesn't respect number of runs #8166
  • Removes confusion between schedule_interval and _schedule_interval

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


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.

@kaxil kaxil added this to the Airflow 1.10.11 milestone Apr 9, 2020
@kaxil kaxil requested a review from ashb April 9, 2020 01:07
@kaxil kaxil requested a review from potiuk April 9, 2020 01:07
@kaxil
Copy link
Copy Markdown
Member Author

kaxil commented Apr 9, 2020

Please feel free to suggest a better name for the PR title/commit which looks good for the Changelog.

"""
return self._get_is_paused()

@property
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Maybe @cached_property is better ?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe, the downside to cached_property would be if dag.schedule_interval is changed, this parameter would be out of sync.

We could do it as:

    @property
    def normalized_schedule_interval(self) -> Optional[ScheduleInterval]:
        return self._normalized_schedule_interval(self.schedule_interval)

    @functools.lru_cache(maxsize=1)
    def _normalized_schedule_interval(self, schedule_interval) -> Optional[ScheduleInterval]: 
        ...

Might be overkill.

Copy link
Copy Markdown
Member

@ashb ashb left a comment

Choose a reason for hiding this comment

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

Change looks good. Up to you if you want to do the cached_property/lru_cache or not.

@codecov-io
Copy link
Copy Markdown

Codecov Report

Merging #8225 into master will decrease coverage by 0.72%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8225      +/-   ##
==========================================
- Coverage   88.31%   87.58%   -0.73%     
==========================================
  Files         936      936              
  Lines       45272    45319      +47     
==========================================
- Hits        39980    39694     -286     
- Misses       5292     5625     +333     
Impacted Files Coverage Δ
airflow/models/dag.py 91.68% <100.00%> (+0.03%) ⬆️
airflow/models/dagbag.py 89.83% <100.00%> (ø)
...flow/providers/apache/cassandra/hooks/cassandra.py 21.25% <0.00%> (-72.50%) ⬇️
...w/providers/apache/hive/operators/mysql_to_hive.py 35.84% <0.00%> (-64.16%) ⬇️
airflow/kubernetes/volume_mount.py 44.44% <0.00%> (-55.56%) ⬇️
airflow/providers/postgres/operators/postgres.py 47.82% <0.00%> (-52.18%) ⬇️
airflow/providers/redis/operators/redis_publish.py 50.00% <0.00%> (-50.00%) ⬇️
airflow/kubernetes/volume.py 52.94% <0.00%> (-47.06%) ⬇️
airflow/providers/mongo/sensors/mongo.py 53.33% <0.00%> (-46.67%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0.00%> (-45.08%) ⬇️
... and 15 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 9468187...99e10a7. Read the comment docs.

@kaxil kaxil merged commit 0ccd5b9 into apache:master Apr 9, 2020
@kaxil kaxil deleted the schedule_interal_norm branch April 9, 2020 18:45
@KostyaEsmukov
Copy link
Copy Markdown
Contributor

@kaxil thank you for this fix! I have backported it to 1.10 and applied on my installation (instead of a patch in #8166 (comment)) and can confirm that it indeed fixes the issue.

Will this be backported to v1-10?

@kaxil
Copy link
Copy Markdown
Member Author

kaxil commented Apr 11, 2020

Thanks for confirming @KostyaEsmukov

Yes this will be backported to v1-10 and would
be available in 1.10.11 :)

@KostyaEsmukov
Copy link
Copy Markdown
Contributor

Great, thank you!

kaxil added a commit that referenced this pull request Apr 14, 2020
@kaxil kaxil added the type:improvement Changelog: Improvements label Jul 1, 2020
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Task Duration Bug: Doesn't respect number of runs

4 participants