Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 162 additions & 1 deletion airflow/providers/http/hooks/http.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,16 +17,23 @@
# under the License.
from __future__ import annotations

from typing import Any, Callable
import asyncio
from typing import TYPE_CHECKING, Any, Callable

import aiohttp
import requests
import tenacity
from aiohttp import ClientResponseError
from asgiref.sync import sync_to_async
from requests.auth import HTTPBasicAuth
from requests_toolbelt.adapters.socket_options import TCPKeepAliveAdapter

from airflow.exceptions import AirflowException
from airflow.hooks.base import BaseHook

if TYPE_CHECKING:
from aiohttp.client_reqrep import ClientResponse


class HttpHook(BaseHook):
"""
Expand Down Expand Up @@ -246,3 +253,157 @@ def test_connection(self):
return True, "Connection successfully tested"
except Exception as e:
return False, str(e)


class HttpAsyncHook(BaseHook):
"""
Interact with HTTP servers using Python Async.

:param method: the API method to be called
:param http_conn_id: http connection id that has the base
API url i.e https://www.google.com/ and optional authentication credentials. Default
headers can also be specified in the Extra field in json format.
:param auth_type: The auth type for the service
"""

conn_name_attr = "http_conn_id"
default_conn_name = "http_default"
conn_type = "http"
hook_name = "HTTP"
Comment thread
sunank200 marked this conversation as resolved.
Resolved

def __init__(
self,
method: str = "POST",
http_conn_id: str = default_conn_name,
auth_type: Any = aiohttp.BasicAuth,
retry_limit: int = 3,
retry_delay: float = 1.0,
Comment on lines +279 to +280

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe better to have exponential backoff retry?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you elaborate why exponential backoff retry would be better suited here?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For unknown pattern of usage exponential backoff usual is a better:

  • It add throttling for requests
  • If service temporary down more chance that with exponential backoff request finish successfully in the end.

) -> None:
self.http_conn_id = http_conn_id
self.method = method.upper()
self.base_url: str = ""
self._retry_obj: Callable[..., Any]
self.auth_type: Any = auth_type
if retry_limit < 1:
raise ValueError("Retry limit must be greater than equal to 1")
self.retry_limit = retry_limit
self.retry_delay = retry_delay

async def run(
self,
endpoint: str | None = None,
data: dict[str, Any] | str | None = None,
headers: dict[str, Any] | None = None,
extra_options: dict[str, Any] | None = None,
) -> "ClientResponse":
r"""
Performs an asynchronous HTTP request call

:param endpoint: the endpoint to be called i.e. resource/v1/query?
:param data: payload to be uploaded or request parameters
:param headers: additional headers to be passed through as a dictionary
:param extra_options: Additional kwargs to pass when creating a request.
For example, ``run(json=obj)`` is passed as ``aiohttp.ClientSession().get(json=obj)``
"""
extra_options = extra_options or {}

# headers may be passed through directly or in the "extra" field in the connection
# definition
_headers = {}
auth = None

if self.http_conn_id:
Comment on lines +314 to +315

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it intend to get connection for every run call?

I see that the same behaviour exists in Requests-based hook but I don't think this is a good approach, most of the time we would spend for get connection.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And also don't know what happen when multiple different Trigger's will try to call sync_to_async simultaneously?

  • Is is thread safety?
  • Or is all of them would run sequentially?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taragolis when we call sync_to_async simultaneously these are concurrent calls. This is not multithreading.sync_to_async lets async code call a synchronous function, which is run in a threadpool and control returned to the async coroutine when the synchronous function completes. So it concurrency than multithreading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, I found a time for check this magic. As I could see sync_to_async run in ThreadPoolExecutor which is use internally threads. However for avoid any thread related issues it run in pool with single worker if thread_sensitive=True.

https://github.com/django/asgiref/blob/730d865506adf565d53e5a67cceb9f796dc472ba/asgiref/sync.py#L345-L346

It will run all functions/methods decorated by sync_to_async sequentially one after another as result no concurrency here, and one function which hung, for example due to infinity timeout to some service which do not send response, would block others

That is some simple sample

import asyncio
import datetime
import time
from asgiref.sync import sync_to_async


@sync_to_async
def task(value):
    print(f"{datetime.datetime.utcnow().isoformat()}, sync_to_async start {value!r}")
    time.sleep(3)
    print(f"{datetime.datetime.utcnow().isoformat()}, sync_to_async end {value!r}")

async def async_task(value):
    print(f"{datetime.datetime.utcnow().isoformat()}, async_task start {value!r}")
    await asyncio.sleep(3)
    print(f"{datetime.datetime.utcnow().isoformat()}, async_task end {value!r}")

async def main():
    await asyncio.gather(
        asyncio.create_task(task(1)),
        asyncio.create_task(task(2)),
        asyncio.create_task(async_task(3)),
        asyncio.create_task(async_task(4)),
        asyncio.create_task(task(5)),
        asyncio.create_task(async_task(6)),
    )

if __name__ == "__main__":
    asyncio.run(main())

@sunank200 sunank200 Feb 13, 2023 •

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Taragolis we have already used sync_to_async in various airflow provider's code base currently as well like: https://github.com/apache/airflow/blob/main/airflow/providers/google/common/hooks/base_google.py#L629

@andrewgodwin @kaxil WDYT?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main point that sync_to_async decorated functions run in some kind of sequential queue (or otherwise it not thread safe), as result It may lead some issues with thread safety.

Code which run inside sync_to_async not lock native async code but lock execution other sync_to_async decorated code it is demonstrated in my example. The same happen in triggerer airflow service when single hanged sync_to_async decorated function might cause that one of triggered would have huge queue for that code, an almost every single Trigger call sync_to_async.

So, unfortunetly ,"we have already used sync_to_async in various airflow provider's code base currently as well like" not make things better rather than create some kind of time bomb.

I don't mean that you doing a bad job and all this sync_to_async decorated functions added for a good purpose, and in general it nice to have more deferrable tasks out-of-the-box however the main problem that Airflow core not ready to operate with Deferrable tasks which need to have access to Core components, such as Connections, Variables, XCom and etc.

conn = await sync_to_async(self.get_connection)(self.http_conn_id)
Comment thread
sunank200 marked this conversation as resolved.
Resolved

if conn.host and "://" in conn.host:
self.base_url = conn.host
else:
# schema defaults to HTTP
schema = conn.schema if conn.schema else "http"
host = conn.host if conn.host else ""
self.base_url = schema + "://" + host

if conn.port:
self.base_url = self.base_url + ":" + str(conn.port)
if conn.login:
auth = self.auth_type(conn.login, conn.password)
if conn.extra:
try:
_headers.update(conn.extra_dejson)
except TypeError:
self.log.warning("Connection to %s has invalid extra field.", conn.host)
if headers:
_headers.update(headers)

if self.base_url and not self.base_url.endswith("/") and endpoint and not endpoint.startswith("/"):
url = self.base_url + "/" + endpoint
else:
url = (self.base_url or "") + (endpoint or "")

async with aiohttp.ClientSession() as session:
if self.method == "GET":
request_func = session.get
elif self.method == "POST":
request_func = session.post
elif self.method == "PATCH":
request_func = session.patch
elif self.method == "HEAD":
request_func = session.head
elif self.method == "PUT":
request_func = session.put
elif self.method == "DELETE":
request_func = session.delete
elif self.method == "OPTIONS":
request_func = session.options
else:
raise AirflowException(f"Unexpected HTTP Method: {self.method}")
Comment thread
sunank200 marked this conversation as resolved.
Resolved

attempt_num = 1
while True:
response = await request_func(
url,
json=data if self.method in ("POST", "PATCH") else None,
params=data if self.method == "GET" else None,
headers=headers,
auth=auth,
**extra_options,
)
try:
response.raise_for_status()
return response
except ClientResponseError as e:
self.log.warning(
"[Try %d of %d] Request to %s failed.",
attempt_num,
self.retry_limit,
url,
)
if not self._retryable_error_async(e) or attempt_num == self.retry_limit:
self.log.exception("HTTP error with status: %s", e.status)
# In this case, the user probably made a mistake.
# Don't retry.
raise AirflowException(f"{e.status}:{e.message}")

attempt_num += 1
await asyncio.sleep(self.retry_delay)

def _retryable_error_async(self, exception: ClientResponseError) -> bool:
"""
Determines whether or not an exception that was thrown might be successful
on a subsequent attempt.

It considers the following to be retryable:
- requests_exceptions.ConnectionError
- requests_exceptions.Timeout
Comment thread
sunank200 marked this conversation as resolved.
Resolved
- anything with a status code >= 500

Most retryable errors are covered by status code >= 500.
"""
if exception.status == 429:
# don't retry for too Many Requests
return False
if exception.status == 413:
# don't retry for payload Too Large
return False

return exception.status >= 500
Comment thread
sunank200 marked this conversation as resolved.
Resolved
2 changes: 2 additions & 0 deletions airflow/providers/http/provider.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ dependencies:
# release it as a requirement for airflow
- requests>=2.26.0
- requests_toolbelt
- aiohttp
- asgiref

integrations:
- integration-name: Hypertext Transfer Protocol (HTTP)
Expand Down
2 changes: 2 additions & 0 deletions generated/provider_dependencies.json
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,8 @@
},
"http": {
"deps": [
"aiohttp",
"asgiref",
"requests>=2.26.0",
"requests_toolbelt"
],
Expand Down
1 change: 1 addition & 0 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -404,6 +404,7 @@ def write_version(filename: str = str(AIRFLOW_SOURCES_ROOT / "airflow" / "git_ve
"twine",
"wheel",
"yamllint",
"aioresponses",
]


Expand Down
95 changes: 94 additions & 1 deletion tests/providers/http/hooks/test_http.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
from __future__ import annotations

import json
import logging
import os
from collections import OrderedDict
from http import HTTPStatus
Expand All @@ -26,11 +27,12 @@
import pytest
import requests
import tenacity
from aioresponses import aioresponses
from requests.adapters import Response

from airflow.exceptions import AirflowException
from airflow.models import Connection
from airflow.providers.http.hooks.http import HttpHook
from airflow.providers.http.hooks.http import HttpAsyncHook, HttpHook


def get_airflow_connection(unused_conn_id=None):
Expand Down Expand Up @@ -392,3 +394,94 @@ def test_keep_alive_disabled(self):


send_email_test = mock.Mock()


@pytest.fixture
def aioresponse():
"""
Creates an mock async API response.
This comes from a mock library specific to the aiohttp package:
https://github.com/pnuckowski/aioresponses

"""
with aioresponses() as async_response:
yield async_response


@pytest.mark.asyncio
async def test_do_api_call_async_non_retryable_error(aioresponse):
"""Test api call asynchronously with non retryable error."""
hook = HttpAsyncHook(method="GET")
aioresponse.get("http://httpbin.org/non_existent_endpoint", status=400)

with pytest.raises(AirflowException) as exc, mock.patch.dict(
"os.environ",
AIRFLOW_CONN_HTTP_DEFAULT="http://httpbin.org/",
):
await hook.run(endpoint="non_existent_endpoint")

assert str(exc.value) == "400:Bad Request"


@pytest.mark.asyncio
async def test_do_api_call_async_retryable_error(caplog, aioresponse):
"""Test api call asynchronously with retryable error."""
caplog.set_level(logging.WARNING, logger="airflow.providers.http.hooks.http")
hook = HttpAsyncHook(method="GET")

with pytest.raises(AirflowException) as exc, mock.patch.dict(
"os.environ",
AIRFLOW_CONN_HTTP_DEFAULT="http://httpbin.org/",
):
await hook.run(endpoint="non_existent_endpoint")

assert str(exc.value) == "500:Internal Server Error"
assert "[Try 3 of 3] Request to http://httpbin.org/non_existent_endpoint failed" in caplog.text


@pytest.mark.asyncio
async def test_do_api_call_async_unknown_method():
"""Test api call asynchronously for unknown method."""
hook = HttpAsyncHook(method="NOPE")
json = {
"existing_cluster_id": "xxxx-xxxxxx-xxxxxx",
}

with pytest.raises(AirflowException) as exc:
await hook.run(endpoint="non_existent_endpoint", data=json)

assert str(exc.value) == "Unexpected HTTP Method: NOPE"


@pytest.mark.asyncio
async def test_async_post_request(aioresponse):
"""Test api call asynchronously for POST request."""
hook = HttpAsyncHook()

aioresponse.post(
"http://test:8080/v1/test",
status=200,
payload='{"status":{"status": 200}}',
reason="OK",
)

with mock.patch("airflow.hooks.base.BaseHook.get_connection", side_effect=get_airflow_connection):
resp = await hook.run("v1/test")
assert resp.status == 200


@pytest.mark.asyncio
async def test_async_post_request_with_error_code(aioresponse):
"""Test api call asynchronously for POST request with error."""
hook = HttpAsyncHook()

aioresponse.post(
"http://test:8080/v1/test",
status=418,
payload='{"status":{"status": 418}}',
reason="I am teapot",
)

with mock.patch("airflow.hooks.base.BaseHook.get_connection", side_effect=get_airflow_connection):
with pytest.raises(AirflowException):
await hook.run("v1/test")