[airbyte-cdk] - Integrate `HttpClient` into `HttpRequester` #38906

pnilan · 2024-06-03T23:47:14Z

What

Moves request preparation and sending functionality from HttpRequester to HttpClient
Updates HttpClient for compatibility with low-code
- Handles a list of backoff_strategies
- Updates logic for determining user_defined_backoff_time to be compatible with list of backoff_strategie
Updated ErrorHandler interface to enforce max_retries and max_time properties.
Updates Requester interface, removing interpret_response method. This logic is handled by the ErrorHandler class.

How

Update is to align the low-code CDK interfaces with common Python CDK interfaces and to provide sensible error defaults for response actions and failure types for all low-code connectors.

Closes https://github.com/airbytehq/airbyte-internal-issues/issues/7864

Recommended Reading Order

http_client.py
model_to_component_factory
http_requester.py
requester.py
default_error_mapping

Notes

Updated default error mapping to include InvalidSchema and InvalidURL exceptions

…w error handling flow

…and requester to align with python cdk error handler and backoff strategy. Removes `ResponseStatus`.

…tiple

…Removes response status unit tests.

…n preference

…pping

…od name

…dler

…g with previous behavior

maxi297

I think there are still issues with the interface. Can you make sure this is not me missing something here and reply to my comments?

maxi297 · 2024-06-13T19:04:13Z

airbyte-cdk/python/airbyte_cdk/sources/declarative/requesters/http_requester.py

-from airbyte_cdk.sources.streams.http.http import BODY_REQUEST_METHODS
-from airbyte_cdk.sources.streams.http.rate_limiting import http_client_default_backoff_handler, user_defined_backoff_handler
+from airbyte_cdk.sources.streams.http import HttpClient
+from airbyte_cdk.sources.streams.http.error_handlers import ErrorHandler


I'm not sure I get the typing here. It feels like what is being passed to HttpRequested from model_to_component_factory is airbyte_cdk.sources.declarative.requesters.error_handlers.error_handler.ErrorHandler. However, this is another error handler entirely. Is MyPy complaining about the types not matching? It feels like it's only working because we've made the interfaces matches but if there is a change in one, the other might not be updated though.

Following up for visibility: Maxime and I met and we realized the disconnect was because I had made changes in this PR: #38743 which make the updates to the declarative interface and implementations.

The declarative backoffs, error_handlers now inherit from the Python CDK interfaces.

maxi297 · 2024-06-13T19:56:49Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http_client.py

+    def _factor(self) -> float:
+        factor: Optional[float] = None
+        for backoff_strategy in self._backoff_strategies:
+            if hasattr(backoff_strategy, "retry_factor") and backoff_strategy.retry_factor is not None:


It seems like we are still relying on the declarative interfaces here. Should we have this factor as part of the airbyte_cdk.sources.streams.http package?

Follow-up comment: The hasattr exists here because retry_factor is not enforced by the interface and is only used by exponential backoffs.

What it means to be is that we got our abstractions wrong and we are at risk of a breaking change once we have to update this concept.

The easy solution I see is to have BackoffHandler implement def handle(?) -> Callable[[request, request_kwargs, log_formatter], Response] or something like that and stop exposing max_tries, max_time and factor. This though is a bigger change today. I'm fine with pushing this breaking change for later assuming the risk of occurrence is low and the blast radius is somewhat contained

I agree with Maxime that the abstraction looks off. Why can't we rely on the backoff strategie's backoff_time to determine how long to wait before retrying?

The declarative framework already supports exponential backoff without needing to leak the factor abstraction to the requester so we should be able to do the same here

womp. looking at the http requester again, I see that the factor is indeed leaking because of the default value https://github.com/airbytehq/airbyte/blob/master/airbyte-cdk/python/airbyte_cdk/sources/declarative/requesters/http_requester.py#L504C9-L504C24

I think it's more than default value here: it is the fact that we can have multiple backoff strategies that relies on different parameters that we expose. Hence, the implementation of the backoff strategy are not well encapsulated. We could encapsulate that in the HttpClient if we wanted

@tolik0 @pnilan is there a reason we couldn't just use the user_backoff_handler?

I wouldn't expect the default to be required since a backoff strategy should always be defined

maxi297

I would like the approval from someone that has more context (I'm not aware of all the changes here) but from my perspective, there is just the testing to update

maxi297 · 2024-06-14T19:05:05Z

airbyte-cdk/python/unit_tests/connector_builder/test_connector_builder_handler.py

@@ -657,10 +657,22 @@ def test_create_source():

    source = create_source(config, limits)

+    max_retries = 0


Is it safe enough to rely on assert http_client._disable_retries? Else, it feels like we are copy/pasting the logic of
HttpClient. _max_retries which means potentially duplication of work every time the logic change as this will need to be updated

Good catch. I added in that logic before consolidating it in the http_client._max_retries method.

Honestly I don't think this assertion is even worth keeping around since it's already tested via http_client.

I don't think this is enough. We need to make sure the Connector Builder disable retries too

Yeah, fair point. Added a separate test to specifically test for this.

maxi297 · 2024-06-14T19:07:10Z

airbyte-cdk/python/unit_tests/sources/streams/http/test_http_client.py

@@ -391,3 +401,57 @@ def update_response(*args, **kwargs):

    assert http_client._session.send.call_count == call_count
    assert returned_response == valid_response
+
+
+def test_disable_retries():


These tests assert on private parameters. If we change the internal logic without changing the behavior, those will break. Should we instead do a test with requests_mock that ensure that we perform only one query in case of failure? The same question applies to the tests below

maxi297 · 2024-06-14T19:08:34Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http_client.py

@@ -136,34 +147,92 @@ def _create_prepared_request(

        return prepared_request

-    def _send_with_retry(self, request: requests.PreparedRequest, request_kwargs: Mapping[str, Any]) -> requests.Response:
+    def _max_retries(self) -> int:


That assumes that self._error_handler.max_retries is a constant in our current implementations. I think it is but I just wanted to point that out

maxi297 · 2024-06-14T19:13:22Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http_client.py

+    def _factor(self) -> float:
+        factor: Optional[float] = None
+        for backoff_strategy in self._backoff_strategies:
+            if hasattr(backoff_strategy, "retry_factor") and backoff_strategy.retry_factor is not None:


What it means to be is that we got our abstractions wrong and we are at risk of a breaking change once we have to update this concept.

The easy solution I see is to have BackoffHandler implement def handle(?) -> Callable[[request, request_kwargs, log_formatter], Response] or something like that and stop exposing max_tries, max_time and factor. This though is a bigger change today. I'm fine with pushing this breaking change for later assuming the risk of occurrence is low and the blast radius is somewhat contained

…ry is true

…t made

girarda

one small comment on the cached property and a larger question about why the factor concept leaks from the exponential error handling to the http client. That's not necessary in the existing requester so it should be necessary in the client either

girarda · 2024-06-17T21:12:05Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http_client.py

@@ -136,34 +147,92 @@ def _create_prepared_request(

        return prepared_request

-    def _send_with_retry(self, request: requests.PreparedRequest, request_kwargs: Mapping[str, Any]) -> requests.Response:
+    def _max_retries(self) -> int:


this assumes all implementations of max_retries are constant. Since ErrorHandler is an interface that can be customized by any connector, I don't think we can make that assumption

girarda · 2024-06-17T21:14:25Z

airbyte-cdk/python/airbyte_cdk/sources/streams/http/http_client.py

+    def _factor(self) -> float:
+        factor: Optional[float] = None
+        for backoff_strategy in self._backoff_strategies:
+            if hasattr(backoff_strategy, "retry_factor") and backoff_strategy.retry_factor is not None:


I agree with Maxime that the abstraction looks off. Why can't we rely on the backoff strategie's backoff_time to determine how long to wait before retrying?

The declarative framework already supports exponential backoff without needing to leak the factor abstraction to the requester so we should be able to do the same here

…y to exponential backoff

pnilan · 2024-06-17T22:08:19Z

one small comment on the cached property and a larger question about why the factor concept leaks from the exponential error handling to the http client. That's not necessary in the existing requester so it should be necessary in the client either

Removed reference to the factor property within http_client. Updated the ExponentialBackoffStrategy to only use this value internally to calc the backoff time and return it. The backoff handler has a default factor value of 1.

girarda

this is great! good job @pnilan !

pnilan added 30 commits May 23, 2024 08:58

Update reference for ResponseAction

e09401c

Updates ErrorHandler, BackoffStrategy, and HttpRequester for ne…

3d4037a

…w error handling flow

Update backoff strategies for new format

b49b0db

Updates factory method to provide default http response filter

2f02df9

Update composite handler to include valid max_retries

1c0848e

Update error handler, http_requester, and model_to_componet_factory, …

be1c8bf

…and requester to align with python cdk error handler and backoff strategy. Removes `ResponseStatus`.

Updated tests for default_error_handler

d79e994

Add method for determining optimal error resolution when provided mul…

5c629d8

…tiple

Update default error handler and composite error handler unit tests. …

2bd6c60

…Removes response status unit tests.

Updates error handlers to maintain current behavior on response actio…

1523bae

…n preference

Updates error handler tests to expected behavior

d8f4857

Updates reference to ResponseAction

be01417

Update http_response_filter tests to reflect updated default error ma…

a656980

…pping

Update HttpResponseFilter to prioritize user inputted error messages

153eea5

Update declarative error handlers tests for updated backoff_time meth…

0a33f2f

…od name

chore: format code

2dc8a89

Update http_requester and associated tests

3a93bbf

chore: lint

6c3315c

Merge branch 'master' into pnilan/airbyte-cdk-low-code-http-error-han…

f737791

…dler

Update http requester to return response if no error handler, alignin…

bef56d2

…g with previous behavior

mypy type checking

572933e

chore: format code

0cc790d

revert source-harvest files

42f9fd7

Updates poetry.lock

d231eff

Update wait_until_header tests for MagicMock spec

546c1d2

Update test for http response filter for correct behavior

9740aa6

Update variable type

1467382

Update conditional for type checking

06258b2

Update http response filter test

a3cdc2e

Update tests

f5fdf7d

pnilan requested a review from tolik0 June 12, 2024 23:08

pnilan added 2 commits June 12, 2024 16:12

chore: format code

d73cebc

Conform error handler and backoff strategy interfaces to python cdk

7db87c3

pnilan requested a review from maxi297 June 13, 2024 18:29

maxi297 reviewed Jun 13, 2024

View reviewed changes

pnilan requested a review from maxi297 June 14, 2024 17:55

Remove type ignore for composite error handler

32b2e1e

maxi297 reviewed Jun 14, 2024

View reviewed changes

pnilan added 2 commits June 14, 2024 14:02

Update http_client._send_with_retry to bypass handlers if disable_ret…

7167ac2

…ry is true

update tests for mocked responses

f236be5

pnilan requested a review from maxi297 June 14, 2024 21:45

pnilan added 2 commits June 16, 2024 16:17

Updates test_connector_builder_handler to verify only one send reques…

3fab5f9

…t made

chore: format code

0d8088a

tolik0 approved these changes Jun 17, 2024

View reviewed changes

pnilan removed the request for review from a team June 17, 2024 15:29

octavia-squidington-iv requested a review from a team June 17, 2024 15:46

pnilan requested a review from girarda June 17, 2024 21:02

girarda requested changes Jun 17, 2024

View reviewed changes

removes factor reference from http_client -> only reference internall…

70276b9

…y to exponential backoff

pnilan requested a review from girarda June 17, 2024 23:17

pnilan added 2 commits June 17, 2024 17:06

removed cached_property in favor of @Property

712bd78

remove cached property from import

fc79be2

girarda approved these changes Jun 18, 2024

View reviewed changes

Base automatically changed from pnilan/airbyte-cdk-low-code-http-error-handler to master June 18, 2024 00:41

Merge branch 'master' into pnilan/airbyte-cdk-low-code-http-client

b59317a

vercel bot deployed to Preview June 18, 2024 00:56 View deployment

pnilan enabled auto-merge (squash) June 18, 2024 01:00

pnilan merged commit 18e82d9 into master Jun 18, 2024
30 checks passed

pnilan deleted the pnilan/airbyte-cdk-low-code-http-client branch June 18, 2024 01:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[airbyte-cdk] - Integrate `HttpClient` into `HttpRequester` #38906

[airbyte-cdk] - Integrate `HttpClient` into `HttpRequester` #38906

pnilan commented Jun 3, 2024 •

edited

Loading

maxi297 left a comment

maxi297 Jun 13, 2024

pnilan Jun 14, 2024

maxi297 Jun 13, 2024

pnilan Jun 14, 2024

maxi297 Jun 14, 2024

girarda Jun 17, 2024

girarda Jun 17, 2024

maxi297 Jun 17, 2024

girarda Jun 17, 2024

maxi297 left a comment

maxi297 Jun 14, 2024

pnilan Jun 14, 2024

pnilan Jun 14, 2024

maxi297 Jun 14, 2024

pnilan Jun 14, 2024

maxi297 Jun 14, 2024

pnilan Jun 14, 2024

maxi297 Jun 14, 2024

maxi297 Jun 14, 2024

girarda left a comment

girarda Jun 17, 2024

girarda Jun 17, 2024

pnilan commented Jun 17, 2024

girarda left a comment

		@@ -657,10 +657,22 @@ def test_create_source():

		source = create_source(config, limits)

		max_retries = 0

[airbyte-cdk] - Integrate HttpClient into HttpRequester #38906

[airbyte-cdk] - Integrate HttpClient into HttpRequester #38906

Conversation

pnilan commented Jun 3, 2024 • edited Loading

What

How

Recommended Reading Order

Notes

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxi297 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

girarda left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pnilan commented Jun 17, 2024

girarda left a comment

Choose a reason for hiding this comment

[airbyte-cdk] - Integrate `HttpClient` into `HttpRequester` #38906

[airbyte-cdk] - Integrate `HttpClient` into `HttpRequester` #38906

pnilan commented Jun 3, 2024 •

edited

Loading