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/01/05 22:11:47 UTC
[GitHub] [airflow] AmarEL opened a new pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
AmarEL opened a new pull request #13499:
URL: https://github.com/apache/airflow/pull/13499
Add a new argument for HttpSensor to accept a list of Error codes to continue poking
closes: #13451
---
**^ 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] kaxil commented on a change in pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#discussion_r560160307
##########
File path: airflow/providers/http/sensors/http.py
##########
@@ -29,7 +29,10 @@ class HttpSensor(BaseSensorOperator):
404 Not Found or `response_check` returning False.
HTTP Error codes other than 404 (like 403) or Connection Refused Error
- would fail the sensor itself directly (no more poking).
+ would raise an exception and fail the sensor itself directly (no more poking).
+ To avoid fail the task for other codes than 404, the argument `extra_option`
+ can be passed with the value `{'check_response': False}`. It will make the `response_check`
Review comment:
```suggestion
can be passed with the value ``{'check_response': False}``. It will make the ``response_check``
```
----------------------------------------------------------------
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 #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#issuecomment-754954743
[The Workflow run](https://github.com/apache/airflow/actions/runs/464642629) is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^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 merged pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
kaxil merged pull request #13499:
URL: https://github.com/apache/airflow/pull/13499
----------------------------------------------------------------
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 #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#discussion_r552340549
##########
File path: airflow/providers/http/sensors/http.py
##########
@@ -90,7 +96,8 @@ def __init__(
self.headers = headers or {}
self.extra_options = extra_options or {}
self.response_check = response_check
-
+ self.acceptable_status_code = acceptable_status_code or []
+ self.acceptable_status_code.append(404)
Review comment:
Besides, I think you can achieve the desired effect if you pass your logic as parameter ``response_check``. Have you tried this?
----------------------------------------------------------------
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 a change in pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#discussion_r553054929
##########
File path: airflow/providers/http/sensors/http.py
##########
@@ -90,7 +96,8 @@ def __init__(
self.headers = headers or {}
self.extra_options = extra_options or {}
self.response_check = response_check
-
+ self.acceptable_status_code = acceptable_status_code or []
+ self.acceptable_status_code.append(404)
Review comment:
I agree `response_check` is there for exactly that reason to allow multiple use-cases
----------------------------------------------------------------
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 a change in pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#discussion_r560160446
##########
File path: airflow/providers/http/sensors/http.py
##########
@@ -29,7 +29,10 @@ class HttpSensor(BaseSensorOperator):
404 Not Found or `response_check` returning False.
HTTP Error codes other than 404 (like 403) or Connection Refused Error
- would fail the sensor itself directly (no more poking).
+ would raise an exception and fail the sensor itself directly (no more poking).
+ To avoid fail the task for other codes than 404, the argument `extra_option`
Review comment:
```suggestion
To avoid failing the task for other codes than 404, the argument ``extra_option``
```
----------------------------------------------------------------
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 #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
mik-laj commented on a change in pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#discussion_r552340025
##########
File path: airflow/providers/http/sensors/http.py
##########
@@ -90,7 +96,8 @@ def __init__(
self.headers = headers or {}
self.extra_options = extra_options or {}
self.response_check = response_check
-
+ self.acceptable_status_code = acceptable_status_code or []
+ self.acceptable_status_code.append(404)
Review comment:
This will add a new item every time the operator is initialized. When this operator is initialized multiple times, this element will also be added multiple times.
```
MY_API_ERROR_CODES=[500]
HttpSensor(acceptable_status_code=MY_API_ERROR_CODES)
HttpSensor(acceptable_status_code=MY_API_ERROR_CODES)
assert MY_API_ERROR_CODES = [500, 404, 404]
```
----------------------------------------------------------------
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 #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
github-actions[bot] commented on pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#issuecomment-762828569
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] AmarEL commented on a change in pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
AmarEL commented on a change in pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#discussion_r553058065
##########
File path: airflow/providers/http/sensors/http.py
##########
@@ -90,7 +96,8 @@ def __init__(
self.headers = headers or {}
self.extra_options = extra_options or {}
self.response_check = response_check
-
+ self.acceptable_status_code = acceptable_status_code or []
+ self.acceptable_status_code.append(404)
Review comment:
Yep, it's true. I take a deep look and notice the same now. Sorry
But in order for the `response_check` works for other codes than 404, it's necessary to fill the param `extra_option` with the value `{"check_response": False}`.
There is a reference for it in the hook method `run`
> :param extra_options: additional options to be used when executing the request
> i.e. {'check_response': False} to avoid checking raising exceptions on non
> 2XX or 3XX status codes
So, my changes are not necessary to reach this goal.
I will rewrite the tests for cases with `extra_option` and try to improve the doc section with this information of `{"check_response": False}` since it's an internal optional behavior in the hook.
----------------------------------------------------------------
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] AmarEL commented on pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
AmarEL commented on pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#issuecomment-762818508
@mik-laj @kaxil can you guys take a look again?
I turned the PR into a doc improvement and add a test for the expected behavior
----------------------------------------------------------------
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 a change in pull request #13499: Add a new argument for HttpSensor to accept a list of http status code to Continue Poking
Posted by GitBox <gi...@apache.org>.
kaxil commented on a change in pull request #13499:
URL: https://github.com/apache/airflow/pull/13499#discussion_r560160446
##########
File path: airflow/providers/http/sensors/http.py
##########
@@ -29,7 +29,10 @@ class HttpSensor(BaseSensorOperator):
404 Not Found or `response_check` returning False.
HTTP Error codes other than 404 (like 403) or Connection Refused Error
- would fail the sensor itself directly (no more poking).
+ would raise an exception and fail the sensor itself directly (no more poking).
+ To avoid fail the task for other codes than 404, the argument `extra_option`
Review comment:
```suggestion
To avoid fail the task for other codes than 404, the argument ``extra_option``
```
----------------------------------------------------------------
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