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 2022/02/08 12:01:56 UTC
[GitHub] [airflow] victorphoenix3 opened a new pull request #21423: query status polling logic #19877
victorphoenix3 opened a new pull request #21423:
URL: https://github.com/apache/airflow/pull/21423
Query status polling logic currently polls until the query is not in intermmediate state. It should instead poll until query is not in the terminal state.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] victorphoenix3 commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
victorphoenix3 commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1033441916
@ferruzzi A similar polling function exists for Athena queries as well [[link]](https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/hooks/athena.py#L188). Should I modify that too?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] victorphoenix3 commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
victorphoenix3 commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1060567601
@potiuk I have added the unit tests, excuse the delay. Please review.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #21423:
URL: https://github.com/apache/airflow/pull/21423
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a change in pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#discussion_r802956631
##########
File path: airflow/providers/amazon/aws/hooks/emr.py
##########
@@ -114,6 +114,12 @@ class EmrContainerHook(AwsBaseHook):
"CANCEL_PENDING",
)
SUCCESS_STATES = ("COMPLETED",)
+ TERMINAL_STATES = (
+ "COMPLETED",
+ "FAILED",
+ "CANCELLED",
+ "CANCEL_PENDING",
Review comment:
I don't know the EMR behavior well enough to say. If you feel it's intentional and the right thing to do, then leave it in. Just asking because the name strikes me as an intermediate state.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1034055715
@victorphoenix3 Yeah, it looks like the exact same issue. If you are up for it, then by all means! But I'd say drop it in a fresh PR so you don't tie this one up.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] victorphoenix3 commented on a change in pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
victorphoenix3 commented on a change in pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#discussion_r802326999
##########
File path: airflow/providers/amazon/aws/hooks/emr.py
##########
@@ -114,6 +114,12 @@ class EmrContainerHook(AwsBaseHook):
"CANCEL_PENDING",
)
SUCCESS_STATES = ("COMPLETED",)
+ TERMINAL_STATES = (
+ "COMPLETED",
+ "FAILED",
+ "CANCELLED",
+ "CANCEL_PENDING",
Review comment:
"CANCEL_PENDING" is being treated as a [failure state](https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/hooks/emr.py#L111-L115) for the EMR container sensor, which is why I included it as a terminal state. Should it be removed?
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] victorphoenix3 commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
victorphoenix3 commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1034768292
@ferruzzi Here's the separate [PR](https://github.com/apache/airflow/pull/21488) for athena queries. Both PRs are complete, waiting for your approval. Thanks!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi edited a comment on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
ferruzzi edited a comment on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1035352431
I like it. You'll have to wait for a Committer to approve and merge, but I don't see any issues.
Maybe edit the top post to link to the Athena one as well. A reviewer might see that and take them both at once.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] victorphoenix3 commented on a change in pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
victorphoenix3 commented on a change in pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#discussion_r803512235
##########
File path: airflow/providers/amazon/aws/hooks/emr.py
##########
@@ -114,6 +114,12 @@ class EmrContainerHook(AwsBaseHook):
"CANCEL_PENDING",
)
SUCCESS_STATES = ("COMPLETED",)
+ TERMINAL_STATES = (
+ "COMPLETED",
+ "FAILED",
+ "CANCELLED",
+ "CANCEL_PENDING",
Review comment:
Going by the code [here](https://github.com/apache/airflow/blob/main/airflow/providers/amazon/aws/hooks/emr.py#L111-L115) and [here](https://github.com/apache/airflow/blob/main/tests/providers/amazon/aws/sensors/test_emr_containers.py#L69-L72), I'll leave it in as a terminal state.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1040202112
Is it possible to add a unit test for that one (and rebase it ? ) I am happy to approve it but some unit testing could prevent regressions.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1060664931
The failing tests were unrelated (I am fixing them in a separate branch and updated constraints to get rid of those errors). I rebased the PR to latest main to re-test it.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1040202112
Is it possible to add a unit test for that one (and rebase it ? ) I am happy to approve it but some unit testing could prevent regressions.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] potiuk merged pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
potiuk merged pull request #21423:
URL: https://github.com/apache/airflow/pull/21423
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on a change in pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on a change in pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#discussion_r802050744
##########
File path: airflow/providers/amazon/aws/hooks/emr.py
##########
@@ -114,6 +114,12 @@ class EmrContainerHook(AwsBaseHook):
"CANCEL_PENDING",
)
SUCCESS_STATES = ("COMPLETED",)
+ TERMINAL_STATES = (
+ "COMPLETED",
+ "FAILED",
+ "CANCELLED",
+ "CANCEL_PENDING",
Review comment:
Is "CANCEL_PENDING" really a terminal state?
Looks good to me other than this question.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #21423: query status polling logic #19877
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1032532425
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:
- Pay attention to the quality of your code (flake8, mypy and type annotations). Our [pre-commits]( https://github.com/apache/airflow/blob/main/STATIC_CODE_CHECKS.rst#prerequisites-for-pre-commit-hooks) will help you with that.
- In case of a new feature add useful documentation (in docstrings or in `docs/` directory). Adding a new operator? Check this short [guide](https://github.com/apache/airflow/blob/main/docs/apache-airflow/howto/custom-operator.rst) Consider adding an example DAG that shows how users should use it.
- Consider using [Breeze environment](https://github.com/apache/airflow/blob/main/BREEZE.rst) for testing locally, itโs a heavy docker but it ships with a working Airflow and a lot of integrations.
- Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
- Please follow [ASF Code of Conduct](https://www.apache.org/foundation/policies/conduct) for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
- Be sure to read the [Airflow Coding style]( https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst#coding-style-and-best-practices).
Apache Airflow is a community-driven project and together we are making it better ๐.
In case of doubts contact the developers at:
Mailing List: dev@airflow.apache.org
Slack: https://s.apache.org/airflow-slack
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] ferruzzi commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
ferruzzi commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1035352431
I like it. You'll have to wait for a Committer to approve and merge, but I don't see any issues.
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [airflow] boring-cyborg[bot] commented on pull request #21423: Refactor query status polling logic in EMRContainerHook
Posted by GitBox <gi...@apache.org>.
boring-cyborg[bot] commented on pull request #21423:
URL: https://github.com/apache/airflow/pull/21423#issuecomment-1061750812
Awesome work, congrats on your first merged pull request!
--
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.
To unsubscribe, e-mail: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org