Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[AIRFLOW-6860] Default ignore_first_depends_on_past to True #7610

Merged
merged 2 commits into from
Mar 20, 2020

Conversation

zhongjiajie
Copy link
Member

@zhongjiajie zhongjiajie commented Mar 3, 2020


Issue link: AIRFLOW-6860

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.

@zhongjiajie
Copy link
Member Author

ref #7490 (comment)

@zhongjiajie
Copy link
Member Author

CI sad with unrelated reason

tests/test_config_templates.py ....                                      [  0%]
tests/test_configuration.py ...................................          [  0%]
tests/test_core.py ........................                              [  1%]
tests/test_core_to_contrib.py .......................................... [  1%]
........................................................................ [  3%]
........................................................................ [  4%]
........................................................................ [  6%]
........................................................................ [  7%]
........................................................................ [  8%]
........................................................................ [ 10%]
........................................................................ [ 11%]
........................................................................ [ 12%]
........................................................................ [ 14%]
........................................................................ [ 15%]
........................................................................ [ 16%]
..........................................                               [ 17%]
tests/test_example_dags.py ..                                            [ 17%]
tests/test_example_dags_system.py .Timeout (0:08:00)!
Thread 0x00007f63a8dbf400 (most recent call first):

@zhongjiajie
Copy link
Member Author

It's weird, it pass in my local breeze env, but failed in Travis both two of my PR. #7593

@turbaszek
Copy link
Member

Yeah, sometimes the tests timeouts. I think @mik-laj was investigating it and we think is related to limited resources on CI (memory swap for example).

@zhongjiajie
Copy link
Member Author

@nuclearpinguin Thanks for the clarification, so did it Travis side or ours?

@potiuk
Copy link
Member

potiuk commented Mar 3, 2020

It looks like Travis quirks - it looks like the same tests start to fail more often and then after some time they continue to work more reliably. It' super-difficult trace (and know whose fault it is) and figure out what's going on. This is not a simple "We" vs. Travis.

We already fixed (or workarounded) a number of problems with Travis and we have an outstanding task to migrate to Github Actions but we have other high priority tasks.

@zhongjiajie - Maybe you could help with investigating it or migrating to Github Actions (where we believe things will be more stable)?

@zhongjiajie
Copy link
Member Author

@potiuk Like to help, but I don't have much experience in CI/CD. I could see we already have some discuss in dev-list, and does the related AIP is? https://cwiki.apache.org/confluence/display/AIRFLOW/AIP-23+Migrate+out+of+Travis+CI

@potiuk
Copy link
Member

potiuk commented Mar 3, 2020

Yep. That's the related PR indeed :)

@zhongjiajie zhongjiajie force-pushed the correct_cli_dag_command branch 4 times, most recently from d760d76 to d4db2c9 Compare March 9, 2020 01:57
@zhongjiajie
Copy link
Member Author

@potiuk @nuclearpinguin PTAL, thx.

@ashb
Copy link
Member

ashb commented Mar 9, 2020

Since the original change/PR hasn't been included in a release yet, this should be targeted against the same JIRA

@zhongjiajie
Copy link
Member Author

Since the original change/PR hasn't been included in a release yet, this should be targeted against the same JIRA

Got it

@zhongjiajie zhongjiajie changed the title [AIRFLOW-6976] Correct cli dag command ignore-first-depends-on-past [AIRFLOW-6860] Default ignore_first_depends_on_past to True Mar 9, 2020
@zhongjiajie
Copy link
Member Author

zhongjiajie commented Mar 9, 2020

I use the same JIRA ticket, change PR title and git commit message, and also close this PR original ticket https://issues.apache.org/jira/browse/AIRFLOW-6976

@ashb
Copy link
Member

ashb commented Mar 9, 2020

@zhongjiajie The reason for targetting this against AIRFLOW-6976 and not a new jira is around release-automation: if we have separate Jiras for a "related" fix then it is possible to miss this change if the first PR was backported. By having two commits target a single jira issue our existing release scripts (./dev/airflow-jira) will show both commits need to be pulled in.

@zhongjiajie
Copy link
Member Author

@zhongjiajie The reason for targetting this against AIRFLOW-6976 and not a new jira is around release-automation: if we have separate Jiras for a "related" fix then it is possible to miss this change if the first PR was backported. By having two commits target a single jira issue our existing release scripts (./dev/airflow-jira) will show both commits need to be pulled in.

I haven't noticed that before, thanks Ash for the clarification

@zhongjiajie
Copy link
Member Author

zhongjiajie commented Mar 10, 2020

Travis failed due to job name mistake and fix in #7668, I rebase on master on restart the failing test.

@codecov-io
Copy link

Codecov Report

Merging #7610 into master will decrease coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7610      +/-   ##
==========================================
- Coverage   86.95%   86.67%   -0.28%     
==========================================
  Files         900      900              
  Lines       43605    43605              
==========================================
- Hits        37915    37796     -119     
- Misses       5690     5809     +119
Impacted Files Coverage Δ
airflow/cli/commands/dag_command.py 85.18% <100%> (ø) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 47.18% <0%> (-45.08%) ⬇️
...viders/cncf/kubernetes/operators/kubernetes_pod.py 69.69% <0%> (-25.26%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️

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 c93f82b...42da161. Read the comment docs.

@zhongjiajie
Copy link
Member Author

zhongjiajie commented Mar 10, 2020

Travis happy, could you please take a look? @ashb @potiuk

@zhongjiajie
Copy link
Member Author

UPDATING.md Outdated Show resolved Hide resolved
Co-Authored-By: Felix Uellendall <[email protected]>
@zhongjiajie
Copy link
Member Author

Fix UPDATING.md

@kaxil kaxil merged commit 9575706 into apache:master Mar 20, 2020
@zhongjiajie zhongjiajie deleted the correct_cli_dag_command branch March 20, 2020 11:29
@zhongjiajie
Copy link
Member Author

Thanks everyone

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.

7 participants