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/04/09 14:42:03 UTC
[GitHub] [airflow] zhongjiajie opened a new pull request #8233: Dag tags use
set instead of list
zhongjiajie opened a new pull request #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233
Dag tags store in db without duplicate name in each
dag, but list could contain duplicate elements, so
it's better to change tags attr in Dag from list to
set
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
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] zhongjiajie edited a comment on issue #8233: Dag tags use
set instead of list
Posted by GitBox <gi...@apache.org>.
zhongjiajie edited a comment on issue #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233#issuecomment-612708404
@kaxil Ok, if we just need to change one line, I prefer to do it in https://github.com/apache/airflow/pull/8231 and close this PR
----------------------------------------------------------------
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] zhongjiajie commented on issue #8233: Dag tags use set
instead of list
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233#issuecomment-612708404
Ok, if we just need to change one line, I prefer to do it in https://github.com/apache/airflow/pull/8231 and close this PR
----------------------------------------------------------------
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] BasPH commented on issue #8233: Dag tags use set instead
of list
Posted by GitBox <gi...@apache.org>.
BasPH commented on issue #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233#issuecomment-612366458
@kaxil I think it might be worth changing this call to a set: https://github.com/apache/airflow/blob/master/airflow/models/dag.py#L1532. It seems currently the DagTags are made unique when fetching from the DB.
Other than that, we can close this pull request IMO.
----------------------------------------------------------------
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] zhongjiajie closed pull request #8233: Dag tags use set
instead of list
Posted by GitBox <gi...@apache.org>.
zhongjiajie closed pull request #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233
----------------------------------------------------------------
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] zhongjiajie commented on issue #8233: Dag tags use set
instead of list
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233#issuecomment-612595746
BTW, I submit this when I review bulk sync to database code in https://github.com/apache/airflow/pull/8231
----------------------------------------------------------------
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 #8233: Dag tags use set
instead of list
Posted by GitBox <gi...@apache.org>.
codecov-io commented on issue #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233#issuecomment-611694409
# [Codecov](https://codecov.io/gh/apache/airflow/pull/8233?src=pr&el=h1) Report
> Merging [#8233](https://codecov.io/gh/apache/airflow/pull/8233?src=pr&el=desc) into [master](https://codecov.io/gh/apache/airflow/commit/ad42c4076479fe7420dc329ee8bfd51d9fb0a2ae&el=desc) will **decrease** coverage by `0.29%`.
> The diff coverage is `100.00%`.
[](https://codecov.io/gh/apache/airflow/pull/8233?src=pr&el=tree)
```diff
@@ Coverage Diff @@
## master #8233 +/- ##
==========================================
- Coverage 88.35% 88.05% -0.30%
==========================================
Files 936 936
Lines 45305 45316 +11
==========================================
- Hits 40030 39905 -125
- Misses 5275 5411 +136
```
| [Impacted Files](https://codecov.io/gh/apache/airflow/pull/8233?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [airflow/example\_dags/example\_bash\_operator.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9iYXNoX29wZXJhdG9yLnB5) | `94.44% <ø> (ø)` | |
| [airflow/example\_dags/example\_branch\_operator.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9icmFuY2hfb3BlcmF0b3IucHk=) | `100.00% <ø> (ø)` | |
| [...ample\_dags/example\_branch\_python\_dop\_operator\_3.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9icmFuY2hfcHl0aG9uX2RvcF9vcGVyYXRvcl8zLnB5) | `75.00% <ø> (ø)` | |
| [airflow/example\_dags/example\_complex.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9jb21wbGV4LnB5) | `100.00% <ø> (ø)` | |
| [...w/example\_dags/example\_external\_task\_marker\_dag.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9leHRlcm5hbF90YXNrX21hcmtlcl9kYWcucHk=) | `100.00% <ø> (ø)` | |
| [...irflow/example\_dags/example\_kubernetes\_executor.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9rdWJlcm5ldGVzX2V4ZWN1dG9yLnB5) | `85.00% <ø> (ø)` | |
| [...example\_dags/example\_kubernetes\_executor\_config.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9rdWJlcm5ldGVzX2V4ZWN1dG9yX2NvbmZpZy5weQ==) | `75.00% <ø> (ø)` | |
| [airflow/example\_dags/example\_latest\_only.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9sYXRlc3Rfb25seS5weQ==) | `100.00% <ø> (ø)` | |
| [...w/example\_dags/example\_latest\_only\_with\_trigger.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9sYXRlc3Rfb25seV93aXRoX3RyaWdnZXIucHk=) | `100.00% <ø> (ø)` | |
| [...le\_dags/example\_passing\_params\_via\_test\_command.py](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree#diff-YWlyZmxvdy9leGFtcGxlX2RhZ3MvZXhhbXBsZV9wYXNzaW5nX3BhcmFtc192aWFfdGVzdF9jb21tYW5kLnB5) | `100.00% <ø> (ø)` | |
| ... and [76 more](https://codecov.io/gh/apache/airflow/pull/8233/diff?src=pr&el=tree-more) | |
------
[Continue to review full report at Codecov](https://codecov.io/gh/apache/airflow/pull/8233?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/8233?src=pr&el=footer). Last update [ad42c40...f1b794f](https://codecov.io/gh/apache/airflow/pull/8233?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] kaxil commented on issue #8233: Dag tags use set instead
of list
Posted by GitBox <gi...@apache.org>.
kaxil commented on issue #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233#issuecomment-612643623
> Sorry for reply late due to have cold. @kaxil @BasPH I change dag tags from list to set due to two main reason:
>
> 1. In https://github.com/apache/airflow/blob/6ba672eaab84fd71dc8a6f2dd5791651e5a96c38/airflow/models/dag.py#L1654-L1660
> we have unique key on `name and dag_id` but in https://github.com/apache/airflow/blob/6ba672eaab84fd71dc8a6f2dd5791651e5a96c38/airflow/models/dag.py#L234
> we define `tags` in list(which may contain duplicate element). Since each dag can not contain duplicate tag, should we define `Dag.tags` without duplicate element and using set would be a solution
> 2. Users use tag when they define DAG file with set, them may came to their mind `DAG tags is a set and no duplicate element`. I think will be more clear. I know we will drop the duplicate element by code or database constraint, but maybe avoid it by user define side would be better.
>
> WDYT
I think we can just change the line @BasPH mentioned (https://github.com/apache/airflow/blob/master/airflow/models/dag.py#L1532. ) to a `set`. We can allow user to submit any Iterable (List, Set etc)
----------------------------------------------------------------
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] zhongjiajie commented on issue #8233: Dag tags use set
instead of list
Posted by GitBox <gi...@apache.org>.
zhongjiajie commented on issue #8233: Dag tags use set instead of list
URL: https://github.com/apache/airflow/pull/8233#issuecomment-612595326
Sorry for reply late due to have cold. @kaxil @BasPH I change dag tags from list to set due to two main reason:
1. In https://github.com/apache/airflow/blob/6ba672eaab84fd71dc8a6f2dd5791651e5a96c38/airflow/models/dag.py#L1654-L1660 we have unique key on `name and dag_id` but in https://github.com/apache/airflow/blob/6ba672eaab84fd71dc8a6f2dd5791651e5a96c38/airflow/models/dag.py#L234 we define `tags` in list(which may contain duplicate element). Since each dag can not contain duplicate tag, should we define `Dag.tags` without duplicate element and using set would be a solution
2. Users use tag when they define DAG file with set, them may came to their mind `DAG tags is a set and no duplicate element`. I think will be more clear. I know we will drop the duplicate element by code or database constraint, but maybe avoid it by user define side would be better.
WDYT
----------------------------------------------------------------
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