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-4560] Fix Tez queue parameter name in mapred_queue #5315

Merged

Conversation

aliceabe
Copy link
Contributor

@aliceabe aliceabe commented May 22, 2019

Jira

  • My PR addresses the following Airflow Jira issues and references them in the PR title.

Description

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

The Tez configuration parameter passed is incorrect, it should be tez.queue.name and not tez.job.queue.name.
See Tez documentation https://tez.apache.org/releases/0.9.2/tez-api-javadocs/configs/TezConfiguration.html

Tests

  • My PR adds the following unit tests OR does not need testing for this extremely good reason:

Only fixing a typo

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.
    • All the public functions and the classes in the PR contain docstrings that explain what it does
    • If you implement backwards incompatible changes, please leave a note in the Updating.md so we can assign it to a appropriate release

Code Quality

  • Passes flake8

@krishnabhupatiraju @KevinYang21 @bolkedebruin @wolfier

@codecov-io
Copy link

codecov-io commented May 22, 2019

Codecov Report

Merging #5315 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5315      +/-   ##
==========================================
- Coverage   78.92%   78.91%   -0.01%     
==========================================
  Files         479      479              
  Lines       30098    30098              
==========================================
- Hits        23755    23753       -2     
- Misses       6343     6345       +2
Impacted Files Coverage Δ
airflow/hooks/hive_hooks.py 75.32% <ø> (ø) ⬆️
airflow/models/taskinstance.py 92.08% <0%> (-0.35%) ⬇️

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 b8d8371...2e3e56a. Read the comment docs.

@aliceabe
Copy link
Contributor Author

@KevinYang21 Can you please PTAL and help merge this?

@KevinYang21
Copy link
Member

@aliceabe Thank you for the fix, is this something that we can cover by a unit test?

@aliceabe
Copy link
Contributor Author

@KevinYang21 None of the other queue parameters are currently under unit tests, so it seems out of scope for this diff.

@KevinYang21
Copy link
Member

@aliceabe indeed we don't have unit tests on other parameters but it's hard to justify code change w/o unit test only saying it didn't have test. On the other hand, for this particular change I think it is a small enough change with document supporting it that we can merge it now--having tests around it is still preferable, someone else might commit the same typo again and not aware of they're breaking something.

@KevinYang21 KevinYang21 merged commit 03ee1c3 into apache:master May 23, 2019
@aliceabe
Copy link
Contributor Author

I see, makes sense @KevinYang21 . Thank you!

andriisoldatenko pushed a commit to andriisoldatenko/airflow that referenced this pull request Jul 26, 2019
wmorris75 pushed a commit to modmed/incubator-airflow that referenced this pull request Jul 29, 2019
kaxil pushed a commit that referenced this pull request Nov 21, 2019
eladkal pushed a commit to eladkal/airflow that referenced this pull request Dec 2, 2019
kaxil pushed a commit that referenced this pull request Dec 12, 2019
cong-zhu pushed a commit to cong-zhu/airflow that referenced this pull request Feb 6, 2020
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.

5 participants