Skip to content

Conversation

@turbaszek
Copy link
Member

@turbaszek turbaszek commented Feb 28, 2020

Now we have the DagRunType there's no more need to keep ID_PREFIX and ID_FORMAT_PREFIX used for run_id.


Issue link: AIRFLOW-6954

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.

@turbaszek turbaszek requested review from kaxil and mik-laj February 28, 2020 10:55
@boring-cyborg boring-cyborg bot added the area:Scheduler including HA (high availability) scheduler label Feb 28, 2020
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need this method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hm, it seems to be used only in tests. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Yes. Fstrings are a better idea. It's clearer

Copy link
Member

Choose a reason for hiding this comment

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

image

Copy link
Member

Choose a reason for hiding this comment

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

Can this method create this template by itself? It seems to me that this template does not change and it will always be {DagRunType}__{execution_date}

Copy link
Member Author

Choose a reason for hiding this comment

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

Nearly always, backfill has only one underscore

Copy link
Member

Choose a reason for hiding this comment

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

We need to standardize it if we want to introduce a new column in the table later. I would be happy if this PR cleaned up all the weirdnesses about DagRunType. In the next PR, we will update the database model and queries.

Cleaning up this mess is crucial for performance.

select * from test_runs where run_type=‘{}’
(0.0284, 0.037, 0.025)
select * from test_runs where run_id  like ‘{}%’
(0.0426, 0.0531, 0.0377)
type / like [-33.35%, -28.89%, -32.99%]
(avg, min, max)

Copy link
Member Author

Choose a reason for hiding this comment

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

@mik-laj I agree so I used the double underscore

@turbaszek turbaszek force-pushed the AIRFLOW-6954-remove-if-prefixes branch from 7c6c434 to 3f5c979 Compare February 28, 2020 15:23
@boring-cyborg boring-cyborg bot added the area:webserver Webserver related Issues label Feb 28, 2020
@turbaszek turbaszek force-pushed the AIRFLOW-6954-remove-if-prefixes branch 4 times, most recently from 4d098eb to f407053 Compare February 29, 2020 21:15
@turbaszek turbaszek requested review from mik-laj and potiuk March 1, 2020 14:59
@turbaszek turbaszek requested a review from mik-laj March 6, 2020 15:20
@turbaszek turbaszek force-pushed the AIRFLOW-6954-remove-if-prefixes branch from f4b8837 to 745f40f Compare March 7, 2020 10:35
fixup! [AIRFLOW-6954] Use DagRunType instead of ID_PREFIX in run_id
@turbaszek turbaszek force-pushed the AIRFLOW-6954-remove-if-prefixes branch from 745f40f to 70a0bb7 Compare March 12, 2020 10:39
@codecov-io
Copy link

codecov-io commented Mar 12, 2020

Codecov Report

Merging #7583 into master will decrease coverage by 0.51%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7583      +/-   ##
==========================================
- Coverage   86.98%   86.47%   -0.52%     
==========================================
  Files         904      904              
  Lines       43728    43722       -6     
==========================================
- Hits        38037    37808     -229     
- Misses       5691     5914     +223     
Impacted Files Coverage Δ
airflow/jobs/base_job.py 91.54% <ø> (ø)
airflow/jobs/scheduler_job.py 90.68% <ø> (ø)
airflow/api/common/experimental/mark_tasks.py 95.51% <100.00%> (+0.02%) ⬆️
airflow/api/common/experimental/trigger_dag.py 98.07% <100.00%> (ø)
airflow/jobs/backfill_job.py 92.11% <100.00%> (-0.05%) ⬇️
airflow/models/dagrun.py 95.68% <100.00%> (-0.10%) ⬇️
airflow/ti_deps/deps/dagrun_id_dep.py 100.00% <100.00%> (ø)
airflow/utils/types.py 100.00% <100.00%> (ø)
airflow/www/views.py 76.13% <100.00%> (ø)
...flow/providers/apache/cassandra/hooks/cassandra.py 21.51% <0.00%> (-72.16%) ⬇️
... and 14 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 6cc9d81...70a0bb7. Read the comment docs.

@turbaszek turbaszek merged commit 3e23235 into apache:master Mar 12, 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 area:webserver Webserver related Issues

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants