You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2021/03/24 14:11:47 UTC
[GitHub] [airflow] SamWheating opened a new pull request #14981: Allow override of project_id in DataprocJobBaseOperator
SamWheating opened a new pull request #14981:
URL: https://github.com/apache/airflow/pull/14981
<!--
Thank you for contributing! Please make sure that your code changes
are covered with tests. And in case of new features or big changes
remember to adjust the documentation.
Feel free to ping committers for the review!
In case of existing issue, reference it using one of the following:
closes: #ISSUE
related: #ISSUE
How to write a good git commit message:
http://chris.beams.io/posts/git-commit/
-->
Operators extending the DataprocSubmitJobOperator currently infer the project_id from the GCP connection, which isn't always correct (for example, when using a service account from one Project to submit jobs to a dataproc cluster in a different project).
In our case, this led to 404s when trying to submit jobs, as the operator incorrectly assumed that the dataproc cluster was in the same project as the service account used to submit the job.
Adding an optional override, if the project_id is left unspecified it will fallback to the old behaviour.
I also understand that these operators are now deprecated, so if you don't want to make changes to them you can close this PR.
---
**^ Add meaningful description above**
Read the **[Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines)** for more information.
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#discussion_r600576366
##########
File path: airflow/providers/google/cloud/operators/dataproc.py
##########
@@ -943,9 +947,8 @@ def __init__(
self.job_error_states = job_error_states if job_error_states is not None else {'ERROR'}
self.impersonation_chain = impersonation_chain
-
self.hook = DataprocHook(gcp_conn_id=gcp_conn_id, impersonation_chain=impersonation_chain)
Review comment:
Thats a really good point, but we won't be able to get the project_id from the hook if the hook is not yet instantiated 🤔 We would also have to update the generate_job function in all of the operators which inherit from this one.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#issuecomment-806070101
[The Workflow run](https://github.com/apache/airflow/actions/runs/684088630) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] kaxil commented on pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
kaxil commented on pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#issuecomment-805996130
Please rebase on latest Master, failing doc build should be fixed by https://github.com/apache/airflow/pull/14986
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#issuecomment-805871891
[The Workflow run](https://github.com/apache/airflow/actions/runs/683371880) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
SamWheating commented on pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#issuecomment-808302773
> please consider using `DataprocSubmitJobOperator`. All other operators like pig, spark etc are deprecated and will be removed one day.
I'll pass this along to the owner of the jobs in question. We encountered this issue when migrating DAGs from an out-of-date airflow environment to a much newer one, hence the use of outdated operators.
Is it still worthwhile to fix/improve these deprecated operators? If not, feel free to close this PR @turbaszek.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] github-actions[bot] commented on pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#issuecomment-808907639
The PR is likely OK to be merged with just subset of tests for default Python and Database versions without running the full matrix of tests, because it does not modify the core of Airflow. If the committers decide that the full tests matrix is needed, they will add the label 'full tests needed'. Then you should rebase to the latest master or amend the last commit of the PR, and push it with --force-with-lease.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#issuecomment-807045909
@SamWheating please consider using `DataprocSubmitJobOperator`. All other operators like pig, spark etc are deprecated and will be removed one day.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] SamWheating commented on a change in pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
SamWheating commented on a change in pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#discussion_r600576366
##########
File path: airflow/providers/google/cloud/operators/dataproc.py
##########
@@ -943,9 +947,8 @@ def __init__(
self.job_error_states = job_error_states if job_error_states is not None else {'ERROR'}
self.impersonation_chain = impersonation_chain
-
self.hook = DataprocHook(gcp_conn_id=gcp_conn_id, impersonation_chain=impersonation_chain)
Review comment:
Thats a really good point, but we won't be able to get the project_id from the hook if the hook is not yet instantiated 🤔 We would also have to update the generate_job function in all of the operators which inherit from this one.
Maybe that should be saved for a separate issue? let me know what you think.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on a change in pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#discussion_r600527806
##########
File path: airflow/providers/google/cloud/operators/dataproc.py
##########
@@ -943,9 +947,8 @@ def __init__(
self.job_error_states = job_error_states if job_error_states is not None else {'ERROR'}
self.impersonation_chain = impersonation_chain
-
self.hook = DataprocHook(gcp_conn_id=gcp_conn_id, impersonation_chain=impersonation_chain)
Review comment:
Hook should be initialized in execute method to avoid connection to metadata server at dag loading.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek merged pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #14981:
URL: https://github.com/apache/airflow/pull/14981
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] turbaszek commented on pull request #14981: Allow override of project_id in DataprocJobBaseOperator
Posted by GitBox <gi...@apache.org>.
turbaszek commented on pull request #14981:
URL: https://github.com/apache/airflow/pull/14981#issuecomment-808907466
@SamWheating every fix is good fix. I just wanted to make sure you know about the new operators 👌
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org