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-5889] Fix polling for AWS Batch job status #6765

Merged
merged 1 commit into from
Dec 12, 2019

Conversation

dazza-codes
Copy link
Contributor

@dazza-codes dazza-codes commented Dec 9, 2019

PR target

This PR targets a 1.10.x release for this bug fix

  • should it target the 1.10.test branch instead of master?

Jira

Possible conflicts

Description

  • errors in polling for job status should not fail
    the airflow task when the polling hits an API throttle
    limit; polling should detect those cases and retry a
    few times to get the job status
  • added typing for the BatchProtocol method return
    types, based on the botocore.client.Batch types
  • applied trivial format consistency using black, i.e.
    $ black -t py36 -l 96 {files}

Tests

  • My PR passes existing tests
    • added more unit tests to improve the code coverage
    • refactored some private methods to aid unit tests

Commits

  • My commits all reference Jira issues in their subject lines, and I have squashed multiple commits if they address the same issue. In addition, my commits follow the guidelines from "How to write a good git commit message":

Documentation

  • Not applicable to this bug fix but some pydocs are enhanced by this PR

@dazza-codes dazza-codes force-pushed the aws-batch-poll-exceptions branch 2 times, most recently from 7c29b4d to 7f65c55 Compare December 10, 2019 05:52
@codecov-io
Copy link

codecov-io commented Dec 10, 2019

Codecov Report

Merging #6765 into master will decrease coverage by 0.01%.
The diff coverage is 98.61%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #6765      +/-   ##
=========================================
- Coverage   84.32%   84.3%   -0.02%     
=========================================
  Files         672     672              
  Lines       38179   38210      +31     
=========================================
+ Hits        32195   32214      +19     
- Misses       5984    5996      +12
Impacted Files Coverage Δ
airflow/contrib/operators/awsbatch_operator.py 95.83% <98.61%> (+17.18%) ⬆️
airflow/kubernetes/volume_mount.py 44.44% <0%> (-55.56%) ⬇️
airflow/kubernetes/volume.py 52.94% <0%> (-47.06%) ⬇️
airflow/kubernetes/pod_launcher.py 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 78.2% <0%> (-20.52%) ⬇️
airflow/utils/dag_processing.py 87.42% <0%> (-0.39%) ⬇️
airflow/hooks/dbapi_hook.py 91.52% <0%> (+0.84%) ⬆️
airflow/models/connection.py 68.96% <0%> (+0.98%) ⬆️
airflow/hooks/hive_hooks.py 77.6% <0%> (+1.52%) ⬆️
... and 3 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 0863d41...c52463e. Read the comment docs.

@mik-laj mik-laj added the provider:amazon-aws AWS/Amazon - related issues label Dec 10, 2019
@dazza-codes
Copy link
Contributor Author

dazza-codes commented Dec 10, 2019

The bug fix in this PR should get into a 1.x release, so it might be better if this PR sneaks in before the AIP-21 stuff in #6764 is resolved.

@dazza-codes dazza-codes changed the title [AIRFLOW-5889] Fix polling for AWS Batch job status [WIP][AIRFLOW-5889] Fix polling for AWS Batch job status Dec 10, 2019
@dazza-codes dazza-codes changed the title [WIP][AIRFLOW-5889] Fix polling for AWS Batch job status [AIRFLOW-5889] Fix polling for AWS Batch job status Dec 11, 2019
Copy link
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.

LGTM - one small change requested, then we can merge this.

@dazza-codes dazza-codes changed the title [AIRFLOW-5889] Fix polling for AWS Batch job status [WIP][AIRFLOW-5889] Fix polling for AWS Batch job status Dec 11, 2019
- errors in polling for job status should not fail
  the airflow task when the polling hits an API throttle
  limit; polling should detect those cases and retry a
  few times to get the job status, only failing the task
  when the job description cannot be retrieved
- added typing for the BatchProtocol method return
  types, based on the botocore.client.Batch types
- applied trivial format consistency using black, i.e.
  $ black -t py36 -l 96 {files}
@dazza-codes dazza-codes changed the title [WIP][AIRFLOW-5889] Fix polling for AWS Batch job status [AIRFLOW-5889] Fix polling for AWS Batch job status Dec 12, 2019
@dazza-codes dazza-codes changed the base branch from master to v1-10-test December 12, 2019 02:01
@dazza-codes dazza-codes changed the base branch from v1-10-test to master December 12, 2019 02:02
@ashb ashb dismissed dimberman’s stale review December 12, 2019 11:29

Question answered.

@ashb ashb merged commit 479ee63 into apache:master Dec 12, 2019
@dazza-codes dazza-codes deleted the aws-batch-poll-exceptions branch December 12, 2019 17:56
kaxil pushed a commit that referenced this pull request Dec 17, 2019
…6765)

- errors in polling for job status should not fail
  the airflow task when the polling hits an API throttle
  limit; polling should detect those cases and retry a
  few times to get the job status, only failing the task
  when the job description cannot be retrieved
- added typing for the BatchProtocol method return
  types, based on the botocore.client.Batch types (NOT
  INCLUDED IN THE CHERRY PICK)
- applied trivial format consistency using black, i.e.
  $ black -t py36 -l 96 {files}

(cherry picked from commit 479ee63)
kaxil pushed a commit that referenced this pull request Dec 18, 2019
…6765)

- errors in polling for job status should not fail
  the airflow task when the polling hits an API throttle
  limit; polling should detect those cases and retry a
  few times to get the job status, only failing the task
  when the job description cannot be retrieved
- added typing for the BatchProtocol method return
  types, based on the botocore.client.Batch types (NOT
  INCLUDED IN THE CHERRY PICK)
- applied trivial format consistency using black, i.e.
  $ black -t py36 -l 96 {files}

(cherry picked from commit 479ee63)
ashb pushed a commit that referenced this pull request Dec 19, 2019
…6765)

- errors in polling for job status should not fail
  the airflow task when the polling hits an API throttle
  limit; polling should detect those cases and retry a
  few times to get the job status, only failing the task
  when the job description cannot be retrieved
- added typing for the BatchProtocol method return
  types, based on the botocore.client.Batch types (NOT
  INCLUDED IN THE CHERRY PICK)
- applied trivial format consistency using black, i.e.
  $ black -t py36 -l 96 {files}

(cherry picked from commit 479ee63)
galuszkak pushed a commit to FlyrInc/apache-airflow that referenced this pull request Mar 5, 2020
…pache#6765)

- errors in polling for job status should not fail
  the airflow task when the polling hits an API throttle
  limit; polling should detect those cases and retry a
  few times to get the job status, only failing the task
  when the job description cannot be retrieved
- added typing for the BatchProtocol method return
  types, based on the botocore.client.Batch types
- applied trivial format consistency using black, i.e.
  $ black -t py36 -l 96 {files}
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
provider:amazon-aws AWS/Amazon - related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants