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-6359] spark_submit_hook.py status polling interval config #6909

Merged
merged 6 commits into from
Dec 31, 2019
Merged

[AIRFLOW-6359] spark_submit_hook.py status polling interval config #6909

merged 6 commits into from
Dec 31, 2019

Conversation

tooptoop4
Copy link
Contributor

@tooptoop4 tooptoop4 commented Dec 26, 2019

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:

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

@tooptoop4 tooptoop4 changed the title AIRFLOW-6359 spark_submit_hook.py status polling interval config [AIRFLOW-6359] spark_submit_hook.py status polling interval config Dec 26, 2019
@potiuk
Copy link
Member

potiuk commented Dec 27, 2019

Same here @tooptoop4 -> pre-commit checks are helpful to catch those static-check problems before they are committed/pushed to the repo.

@tooptoop4
Copy link
Contributor Author

travis unrelated?

Copy link
Member

@turbaszek turbaszek left a comment

Choose a reason for hiding this comment

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

I am not sure about this change. In my opinion status_poll_interval should be configured when initialising the hook. It's rather uncommon to have service specific configuration in config.

@potiuk
Copy link
Member

potiuk commented Dec 27, 2019

Agree with @nuclearpinguin. While we already have hive/atlas in there, I think we should get rid of it in favour of configuring the parameters currently in conf via hook parameters. I would love to hear other opinions on that subject - maybe we should have a new JIRA issue ("get rid of non-core extra conf entries) or something like that?

@tooptoop4
Copy link
Contributor Author

tooptoop4 commented Dec 27, 2019

do u mean in my dags i should pass the argument to sparkoperator? (then need to do pr for the operator to use it)

@codecov-io
Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #6909 into master will decrease coverage by 0.3%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6909      +/-   ##
==========================================
- Coverage   84.72%   84.42%   -0.31%     
==========================================
  Files         679      680       +1     
  Lines       38505    38558      +53     
==========================================
- Hits        32623    32551      -72     
- Misses       5882     6007     +125
Impacted Files Coverage Δ
airflow/contrib/hooks/spark_submit_hook.py 82.07% <100%> (-0.43%) ⬇️
airflow/contrib/operators/spark_submit_operator.py 92.85% <100%> (+0.17%) ⬆️
airflow/contrib/hooks/cassandra_hook.py 0% <0%> (-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 45.25% <0%> (-46.72%) ⬇️
airflow/kubernetes/refresh_config.py 50.98% <0%> (-23.53%) ⬇️
...rflow/contrib/operators/kubernetes_pod_operator.py 78.75% <0%> (-20%) ⬇️
airflow/models/__init__.py 91.3% <0%> (-8.7%) ⬇️
airflow/utils/cli.py 71.69% <0%> (-3.04%) ⬇️
... and 18 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 6fffa5b...9bdc64a. Read the comment docs.

@turbaszek
Copy link
Member

do u mean in my dags i should pass the argument to sparkoperator? (then need to do pr for the operator to use it)

Yes, imho that is the way to go

@tooptoop4
Copy link
Contributor Author

@nuclearpinguin updated to not use .cfg

airflow/contrib/hooks/spark_submit_hook.py Outdated Show resolved Hide resolved
airflow/contrib/hooks/spark_submit_hook.py Outdated Show resolved Hide resolved
@tooptoop4
Copy link
Contributor Author

@nuclearpinguin updates made

@tooptoop4
Copy link
Contributor Author

@nuclearpinguin can u pls merge?

@turbaszek turbaszek merged commit 83ba8e6 into apache:master Dec 31, 2019
kaxil pushed a commit that referenced this pull request Jan 23, 2020
…6909)

* AIRFLOW-6359

* pep checks

* make travis CI refire

* new section = bad

* move poll_interval into the operator instead of .cfg

* review comments

(cherry picked from commit 83ba8e6)
potiuk pushed a commit that referenced this pull request Jan 26, 2020
…6909)

* AIRFLOW-6359

* pep checks

* make travis CI refire

* new section = bad

* move poll_interval into the operator instead of .cfg

* review comments

(cherry picked from commit 83ba8e6)
kaxil pushed a commit that referenced this pull request Jan 26, 2020
…6909)

* AIRFLOW-6359

* pep checks

* make travis CI refire

* new section = bad

* move poll_interval into the operator instead of .cfg

* review comments

(cherry picked from commit 83ba8e6)
kaxil pushed a commit that referenced this pull request Feb 3, 2020
…6909)

* AIRFLOW-6359

* pep checks

* make travis CI refire

* new section = bad

* move poll_interval into the operator instead of .cfg

* review comments

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

* AIRFLOW-6359

* pep checks

* make travis CI refire

* new section = bad

* move poll_interval into the operator instead of .cfg

* review comments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants