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/06/25 15:50:07 UTC
[GitHub] [airflow] coopergillan opened a new pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
coopergillan opened a new pull request #9517:
URL: https://github.com/apache/airflow/pull/9517
Since query_execution_id is now returned by execute, it will be xcom
pushed by default, meaning `do_xcom_push` is no longer needed and should
not be mentioned in the docstring.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes)
- [x] Target Github ISSUE in description if exists
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions.
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
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).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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] coopergillan commented on a change in pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#discussion_r450897421
##########
File path: airflow/providers/amazon/aws/operators/athena.py
##########
@@ -28,9 +28,6 @@ class AWSAthenaOperator(BaseOperator):
"""
An operator that submit presto query to athena.
- If ``do_xcom_push`` is True, the QueryExecutionID assigned to the
- query will be pushed to an XCom when it successfuly completes.
Review comment:
[Here is the setting in `BaseOperator` on `master`](https://github.com/apache/airflow/blob/eb403beea2d1035635b7bea24c49b6b964313e51/airflow/models/baseoperator.py#L359). Since operator docstrings usually do not mention super class functionality, I think this could be removed. The behavior should be evident by looking at the super class and understanding that `query_execution_id` is the value in question that will or will not be xcom_pushed.
----------------------------------------------------------------
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] jhtimmins commented on pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
jhtimmins commented on pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#issuecomment-652229569
This change looks good. The earliest history I see for this file is in November 2019, at which point the Athena operator did not make use of `do_xcom_push`. It's not immediately clear if it ever did use the variable, but it isn't now.
----------------------------------------------------------------
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] coopergillan commented on pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
coopergillan commented on pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#issuecomment-654955944
@turbaszek - just rebased and cleaned up the wording a bit. Apologies that I didn't see your approval first. Please let me know if it still stands.
----------------------------------------------------------------
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] coopergillan commented on a change in pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#discussion_r451247522
##########
File path: airflow/providers/amazon/aws/operators/athena.py
##########
@@ -28,9 +28,6 @@ class AWSAthenaOperator(BaseOperator):
"""
An operator that submit presto query to athena.
- If ``do_xcom_push`` is True, the QueryExecutionID assigned to the
- query will be pushed to an XCom when it successfuly completes.
Review comment:
Noted! Just updated the PR description and title to reflect the full context. Thanks for pointing that out!
----------------------------------------------------------------
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 a change in pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#discussion_r448232645
##########
File path: airflow/providers/amazon/aws/operators/athena.py
##########
@@ -28,9 +28,6 @@ class AWSAthenaOperator(BaseOperator):
"""
An operator that submit presto query to athena.
- If ``do_xcom_push`` is True, the QueryExecutionID assigned to the
- query will be pushed to an XCom when it successfuly completes.
Review comment:
I think this is still valid because:
https://github.com/apache/airflow/blob/7a5441836bbc8bbcb0b02bf68dbd93d63fe4ec6a/airflow/models/taskinstance.py#L1035-L1037
----------------------------------------------------------------
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 a change in pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#discussion_r450964004
##########
File path: airflow/providers/amazon/aws/operators/athena.py
##########
@@ -28,9 +28,6 @@ class AWSAthenaOperator(BaseOperator):
"""
An operator that submit presto query to athena.
- If ``do_xcom_push`` is True, the QueryExecutionID assigned to the
- query will be pushed to an XCom when it successfuly completes.
Review comment:
> Since operator docstrings usually do not mention super class functionality, I think this could be removed.
With that I can agree. My point was that this part of the PR description is not true:
>Since query_execution_id is now returned by execute, it will be xcom
pushed by default
----------------------------------------------------------------
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] coopergillan commented on a change in pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
coopergillan commented on a change in pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#discussion_r450894155
##########
File path: airflow/providers/amazon/aws/operators/athena.py
##########
@@ -28,9 +28,6 @@ class AWSAthenaOperator(BaseOperator):
"""
An operator that submit presto query to athena.
- If ``do_xcom_push`` is True, the QueryExecutionID assigned to the
- query will be pushed to an XCom when it successfuly completes.
Review comment:
Got it, that makes sense. This behavior seems to correspond to the super classes and not to this specific class, hence why I don't think it needs to be mentioned in the docstring. Does that make sense?
----------------------------------------------------------------
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 a change in pull request #9517: [AIRFLOW-XXXX] Remove now-inaccurate docstring
Posted by GitBox <gi...@apache.org>.
turbaszek commented on a change in pull request #9517:
URL: https://github.com/apache/airflow/pull/9517#discussion_r448233835
##########
File path: airflow/providers/amazon/aws/operators/athena.py
##########
@@ -28,9 +28,6 @@ class AWSAthenaOperator(BaseOperator):
"""
An operator that submit presto query to athena.
- If ``do_xcom_push`` is True, the QueryExecutionID assigned to the
- query will be pushed to an XCom when it successfuly completes.
Review comment:
If operator's `execute` method returns a `result` it doesn't mean that it will be always saved in XCom table. This will happen only if operator is used with `do_xcom_push=True`
----------------------------------------------------------------
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 #9517: [AIRFLOW-XXXX] Remove unnecessary docstring in AWSAthenaOperator
Posted by GitBox <gi...@apache.org>.
turbaszek merged pull request #9517:
URL: https://github.com/apache/airflow/pull/9517
----------------------------------------------------------------
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