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 2020/09/02 08:00:05 UTC
[GitHub] [airflow] turbaszek opened a new issue #10687: Make region parameter required in Google Dataproc operators and hooks
turbaszek opened a new issue #10687:
URL: https://github.com/apache/airflow/issues/10687
**Description**
Deprecate using `global` as the default `region` value in Google Dataproc operators and hooks. Make the default value `None` and then check if another value was passed. If not then raise a warning and set the value to `global` to preserve backward compatibility.
**Use case / motivation**
This parameter should be required by operators because running in any default (unexpected) region may be seen as undesirable behavior.
**Related Issues**
https://github.com/apache/airflow/pull/10673#discussion_r481057300
----------------------------------------------------------------
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 issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686318881
@mik-laj @jaketf @edejong do you think that we should also unify naming and use `location` instead of `region`?
----------------------------------------------------------------
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] dmitrikuksik edited a comment on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
dmitrikuksik edited a comment on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-687100523
Hi! Can you leave this to me?:) I'll make PR after weekends.
----------------------------------------------------------------
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] jaketf edited a comment on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
jaketf edited a comment on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686848519
@turbaszek
I do not think we should unify naming on location as this would not line up with the REST API or the python client lib.
the right thing to do here is a bit confusing looking at the [REST API docs](https://cloud.google.com/dataproc/docs/reference/rest)
Some dataproc resources (e.g. job / cluster) only have `regions/` endpoints.
Other dataproc resources (workflow templates) have BOTH `regions/` and `locations/` endpoints.
In these cases the python client lib uses full `parent` parameter ([example](https://googleapis.dev/python/dataproc/latest/dataproc_v1beta2/services.html#google.cloud.dataproc_v1beta2.services.workflow_template_service.WorkflowTemplateServiceClient.instantiate_inline_workflow_template)).
This is clearly intentional but I'm not entirely sure the distinction of when one would use `locations/` over the `regions/` endpoint. I imagine a user who understands this difference would want the flexibility to use either endpoint from their airflow DAGs and thus for hook methods / operators that interact with those resources we should provide mutually exclusive `region` AND `location` parameters OR take the same approach as python client lib and use`parent`.
It's my hunch that this locations endpoint is a future proofing thing.
Any given dataproc job runs only on a single dataproc cluster which will in only live in a single zone but we submit jobs to a regional endpoint and can OPTIONALLY specify a zone in the job / cluster config in the request or use [dataproc auto-zone placement](https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/auto-zone) hence why regional endpoints for ultimately zonal resources.
This means "unifying naming on location" will be less explicitly mapping to the API endpoint used for those resources that actually use the region endpoint and this might lead to confusing code like `submit_job(region=self.location,...)`
----------------------------------------------------------------
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] jaketf commented on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
jaketf commented on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686848519
@turbaszek
I do not think we should unify naming on location as this would not line up with the REST API or the python client lib.
the right thing to do here is a bit confusing looking at the [REST API docs](https://cloud.google.com/dataproc/docs/reference/rest)
Some dataproc resources (e.g. job / cluster) only have `regions/` endpoints.
Other dataproc resources (workflow templates) have BOTH `regions/` and `locations/` endpoints.
This is clearly intentional but I'm not entirely sure the distinction of when one would use `locations/` over the `regions/` endpoint. I imagine a user who understands this difference would want the flexibility to use either endpoint from their airflow DAGs and thus for hook methods / operators that interact with those resources we should provide mutually exclusive region AND location parameters. It's my hunch that this locations endpoint is a future proofing thing in case dataproc API allows you to submit jobs to clusters that aren't in GCP regions per say (this is purely my own speculation perhaps we can ask dataproc engineering team for this namespace distinction).
Any given dataproc job runs only on a single dataproc cluster which will in only live in a single zone but we submit jobs to a regional endpoint and can OPTIONALLY specify a zone in the job / cluster config in the request or use [dataproc auto-zone placement](https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/auto-zone) hence why regional endpoints for ultimately zonal resources.
This means "unifying naming on location" will be less explicitly mapping to the API endpoint used for those resources that actually use the region endpoint and this might lead to confusing code like `submit_job(region=self.location,...)`
----------------------------------------------------------------
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 issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-687150930
Awesome @dmitrikuksik, I assigned you 👌
The renaming of `region` -> `location` is not a part of this issue, I just wanted to raise the question as it's a recurring problem and there was no agreement.
----------------------------------------------------------------
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] jaketf edited a comment on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
jaketf edited a comment on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686848519
@turbaszek
I do not think we should unify naming on location as this would not line up with the REST API or the python client lib.
the right thing to do here is a bit confusing looking at the [REST API docs](https://cloud.google.com/dataproc/docs/reference/rest)
Some dataproc resources (e.g. job / cluster) only have `regions/` endpoints.
Other dataproc resources (workflow templates) have BOTH `regions/` and `locations/` endpoints.
In these cases the python client lib uses full `parent` parameter.
This is clearly intentional but I'm not entirely sure the distinction of when one would use `locations/` over the `regions/` endpoint. I imagine a user who understands this difference would want the flexibility to use either endpoint from their airflow DAGs and thus for hook methods / operators that interact with those resources we should provide mutually exclusive region AND location parameters. It's my hunch that this locations endpoint is a future proofing thing.
Any given dataproc job runs only on a single dataproc cluster which will in only live in a single zone but we submit jobs to a regional endpoint and can OPTIONALLY specify a zone in the job / cluster config in the request or use [dataproc auto-zone placement](https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/auto-zone) hence why regional endpoints for ultimately zonal resources.
This means "unifying naming on location" will be less explicitly mapping to the API endpoint used for those resources that actually use the region endpoint and this might lead to confusing code like `submit_job(region=self.location,...)`
----------------------------------------------------------------
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] jaketf commented on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
jaketf commented on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686850162
I will reach out to dataproc eng for their perspective.
----------------------------------------------------------------
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] jaketf edited a comment on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
jaketf edited a comment on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686848519
@turbaszek
I do not think we should unify naming on location as this would not line up with the REST API or the python client lib.
the right thing to do here is a bit confusing looking at the [REST API docs](https://cloud.google.com/dataproc/docs/reference/rest)
Some dataproc resources (e.g. job / cluster) only have `regions/` endpoints.
Other dataproc resources (workflow templates) have BOTH `regions/` and `locations/` endpoints.
In these cases the python client lib uses full `parent` parameter.
This is clearly intentional but I'm not entirely sure the distinction of when one would use `locations/` over the `regions/` endpoint. I imagine a user who understands this difference would want the flexibility to use either endpoint from their airflow DAGs and thus for hook methods / operators that interact with those resources we should provide mutually exclusive `region` AND `location` parameters OR take the same approach as python client lib and use`parent`.
It's my hunch that this locations endpoint is a future proofing thing.
Any given dataproc job runs only on a single dataproc cluster which will in only live in a single zone but we submit jobs to a regional endpoint and can OPTIONALLY specify a zone in the job / cluster config in the request or use [dataproc auto-zone placement](https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/auto-zone) hence why regional endpoints for ultimately zonal resources.
This means "unifying naming on location" will be less explicitly mapping to the API endpoint used for those resources that actually use the region endpoint and this might lead to confusing code like `submit_job(region=self.location,...)`
----------------------------------------------------------------
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 issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
turbaszek commented on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686970197
Thanks @jaketf ! Once we get their response we can decide what to do, because I agree that this can be slighlty confusing for users.
In the meantime, we can still deprecate the region default value 👍
----------------------------------------------------------------
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] jaketf edited a comment on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
jaketf edited a comment on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686848519
@turbaszek
I do not think we should unify naming on location as this would not line up with the REST API or the python client lib.
the right thing to do here is a bit confusing looking at the [REST API docs](https://cloud.google.com/dataproc/docs/reference/rest)
Some dataproc resources (e.g. job / cluster) only have `regions/` endpoints.
Other dataproc resources (workflow templates) have BOTH `regions/` and `locations/` endpoints.
This is clearly intentional but I'm not entirely sure the distinction of when one would use `locations/` over the `regions/` endpoint. I imagine a user who understands this difference would want the flexibility to use either endpoint from their airflow DAGs and thus for hook methods / operators that interact with those resources we should provide mutually exclusive region AND location parameters. It's my hunch that this locations endpoint is a future proofing thing.
Any given dataproc job runs only on a single dataproc cluster which will in only live in a single zone but we submit jobs to a regional endpoint and can OPTIONALLY specify a zone in the job / cluster config in the request or use [dataproc auto-zone placement](https://cloud.google.com/dataproc/docs/concepts/configuring-clusters/auto-zone) hence why regional endpoints for ultimately zonal resources.
This means "unifying naming on location" will be less explicitly mapping to the API endpoint used for those resources that actually use the region endpoint and this might lead to confusing code like `submit_job(region=self.location,...)`
----------------------------------------------------------------
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] dmitrikuksik commented on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
dmitrikuksik commented on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-687899236
I've made my first PR 💪 (https://github.com/apache/airflow/pull/10772). There was `submit` method in dataproc hooks that is going to be deprecated, so I left it without changes. Also, I've ignored `DataprocScaleClusterOperator` in dataproc operators for the same reason, but, added condition to `DataprocJobBaseOperator`. I can see that operators that inherits this class are going to be deprecated, but I guess they are still in use to generate input for new operator `DataprocSubmitJobOperator`.
----------------------------------------------------------------
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] jaketf edited a comment on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
jaketf edited a comment on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-686848519
----------------------------------------------------------------
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] dmitrikuksik commented on issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
dmitrikuksik commented on issue #10687:
URL: https://github.com/apache/airflow/issues/10687#issuecomment-687100523
Hi! Can you leave it on me?:) I'll make PR after weekends.
----------------------------------------------------------------
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 closed issue #10687: Make region parameter required in Google Dataproc operators and hooks
Posted by GitBox <gi...@apache.org>.
turbaszek closed issue #10687:
URL: https://github.com/apache/airflow/issues/10687
----------------------------------------------------------------
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