Skip to content

Conversation

@awelsh93
Copy link
Contributor

@awelsh93 awelsh93 commented Aug 6, 2018

… checking for timeout

Make sure you have checked all steps below.

Jira

Description

  • Here are some details about my PR, including screenshots of any UI changes:

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:
    I ran nosetests /tests/hooks/test_druid_hook.py

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":
    1. Subject is separated from body by a blank line
    2. Subject is limited to 50 characters (not including Jira issue reference)
    3. Subject does not end with a period
    4. Subject uses the imperative mood ("add", not "adding")
    5. Body wraps at 72 characters
    6. Body explains "what" and "why", not "how"

Documentation

  • In case of new functionality, my PR adds documentation that describes how to use it.
    • When adding new operators/hooks/sensors, the autoclass documentation generation needs to be added.

Code Quality

  • Passes git diff upstream/master -u -- "*.py" | flake8 --diff

Copy link
Contributor

@Fokko Fokko left a comment

Choose a reason for hiding this comment

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

Good catch @awelsh93 Thanks

@Fokko Fokko merged commit d12aacd into apache:master Aug 6, 2018
@awelsh93
Copy link
Contributor Author

awelsh93 commented Aug 6, 2018

Hey @Fokko, thanks for your quick response. I've noticed some of the Travis jobs have failed - specifically for test_submit_timeout in tests.hooks.test_druid_hook.TestDruidHook. It seems like a timeout of 0 means this change gets stuck in a loop where sec gets incremented by 0 and hence will never be greater than max_ingestion_time.

What course of action do you recommend?

@Fokko
Copy link
Contributor

Fokko commented Aug 6, 2018

@awelsh93 Thanks for the quick response. Maybe add an assert that the timeout has to be positive. Feel free to change the test as well.

@Fokko
Copy link
Contributor

Fokko commented Aug 6, 2018

@awelsh93 I've reverted the commit for now since it fails the build. Could you create a new PR with the fixed test?

@awelsh93
Copy link
Contributor Author

awelsh93 commented Aug 6, 2018

@Fokko Thanks for your comments, I've created a new pull request (#3710)

Fokko pushed a commit that referenced this pull request Aug 6, 2018
…tly when checking for timeout (#3707)"

This reverts commit d12aacd.
lxneng pushed a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018
lxneng pushed a commit to lxneng/incubator-airflow that referenced this pull request Aug 10, 2018
@awelsh93 awelsh93 deleted the airflow-2860 branch August 14, 2018 09:15
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
aliceabe pushed a commit to aliceabe/incubator-airflow that referenced this pull request Jan 3, 2019
…tly when checking for timeout (apache#3707)"

This reverts commit d12aacd.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants