-
Notifications
You must be signed in to change notification settings - Fork 3.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[airbyte-cdk] Updates Low Code CDK ErrorHandlers and BackoffStrategies to align with Python CDK Interfaces #38743
Changes from all commits
e09401c
3d4037a
b49b0db
2f02df9
1c0848e
be1c8bf
d79e994
5c629d8
2bd6c60
1523bae
d8f4857
be01417
a656980
153eea5
0a33f2f
2dc8a89
3a93bbf
6c3315c
f737791
bef56d2
572933e
0cc790d
42f9fd7
d231eff
546c1d2
9740aa6
1467382
06258b2
a3cdc2e
f5fdf7d
9ca5af1
3a045ce
fec77c3
3949c2a
a52315a
b6d3c14
c010124
9e331ca
34071f4
7cc7ffb
7fb7b4d
ffec9c0
522db58
33eef17
83d598b
cb29218
389cc39
ba759f0
57031fe
ed10145
b6892a9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -1552,7 +1552,6 @@ definitions: | |
type: object | ||
required: | ||
- type | ||
- action | ||
properties: | ||
type: | ||
type: string | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -3,13 +3,11 @@ | |||||||||||||||||||||||||||||||||||
# | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
from dataclasses import InitVar, dataclass | ||||||||||||||||||||||||||||||||||||
from typing import Any, List, Mapping, Union | ||||||||||||||||||||||||||||||||||||
from typing import Any, List, Mapping, Optional, Union | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
import airbyte_cdk.sources.declarative.requesters.error_handlers.response_status as response_status | ||||||||||||||||||||||||||||||||||||
import requests | ||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.declarative.requesters.error_handlers.error_handler import ErrorHandler | ||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.declarative.requesters.error_handlers.response_action import ResponseAction | ||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.declarative.requesters.error_handlers.response_status import ResponseStatus | ||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.streams.http.error_handlers import ErrorHandler | ||||||||||||||||||||||||||||||||||||
from airbyte_cdk.sources.streams.http.error_handlers.response_models import DEFAULT_ERROR_RESOLUTION, ErrorResolution, ResponseAction | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
@dataclass | ||||||||||||||||||||||||||||||||||||
|
@@ -45,19 +43,30 @@ def __post_init__(self, parameters: Mapping[str, Any]) -> None: | |||||||||||||||||||||||||||||||||||
raise ValueError("CompositeErrorHandler expects at least 1 underlying error handler") | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
@property | ||||||||||||||||||||||||||||||||||||
def max_retries(self) -> Union[int, None]: | ||||||||||||||||||||||||||||||||||||
return self.error_handlers[0].max_retries | ||||||||||||||||||||||||||||||||||||
def max_retries(self) -> Optional[int]: | ||||||||||||||||||||||||||||||||||||
return self.error_handlers[0].max_retries # type: ignore # property not defined in ErrorHandler | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
@property | ||||||||||||||||||||||||||||||||||||
def max_time(self) -> Union[int, None]: | ||||||||||||||||||||||||||||||||||||
return max([error_handler.max_time or 0 for error_handler in self.error_handlers]) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
def interpret_response(self, response: requests.Response) -> ResponseStatus: | ||||||||||||||||||||||||||||||||||||
should_retry = ResponseStatus(ResponseAction.FAIL) | ||||||||||||||||||||||||||||||||||||
for retrier in self.error_handlers: | ||||||||||||||||||||||||||||||||||||
should_retry = retrier.interpret_response(response) | ||||||||||||||||||||||||||||||||||||
if should_retry.action == ResponseAction.SUCCESS: | ||||||||||||||||||||||||||||||||||||
return response_status.SUCCESS | ||||||||||||||||||||||||||||||||||||
if should_retry == response_status.IGNORE or should_retry.action == ResponseAction.RETRY: | ||||||||||||||||||||||||||||||||||||
return should_retry | ||||||||||||||||||||||||||||||||||||
return should_retry | ||||||||||||||||||||||||||||||||||||
return max([error_handler.max_time or 0 for error_handler in self.error_handlers]) # type: ignore # property not defined in ErrorHandler | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
def interpret_response(self, response_or_exception: Optional[Union[requests.Response, Exception]]) -> ErrorResolution: | ||||||||||||||||||||||||||||||||||||
matched_error_resolution = None | ||||||||||||||||||||||||||||||||||||
for error_handler in self.error_handlers: | ||||||||||||||||||||||||||||||||||||
matched_error_resolution = error_handler.interpret_response(response_or_exception) | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if not isinstance(matched_error_resolution, ErrorResolution): | ||||||||||||||||||||||||||||||||||||
continue | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if matched_error_resolution.response_action == ResponseAction.SUCCESS: | ||||||||||||||||||||||||||||||||||||
return matched_error_resolution | ||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
if ( | ||||||||||||||||||||||||||||||||||||
matched_error_resolution.response_action == ResponseAction.RETRY | ||||||||||||||||||||||||||||||||||||
or matched_error_resolution.response_action == ResponseAction.IGNORE | ||||||||||||||||||||||||||||||||||||
): | ||||||||||||||||||||||||||||||||||||
return matched_error_resolution | ||||||||||||||||||||||||||||||||||||
if matched_error_resolution: | ||||||||||||||||||||||||||||||||||||
return matched_error_resolution | ||||||||||||||||||||||||||||||||||||
Comment on lines
+58
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The thought behind the current indentation levels/response hierarchies is:
The thought here is we don't want to prematurely fail if there is an error handler with a response filter that has a "better" outcome. IMO this makes a lot of assumptions about the connector dev's intentions, but while the CompositeErrorHandler still exists, I'm trying to maintain its behavior as much as possible. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Then should it be like this?
Why do we not have the same implementation for DefaultErrorHandler? |
||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||
return DEFAULT_ERROR_RESOLUTION |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is interesting. What is the exact error that mypy raises when we don't have this ignore annotation. The
ErrorHandler
interface does have amax_time
property so I'm surprised this gets raised. Not a blocker, but it is curious to know if we can avoid the annotation because the code itself looks fineThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is because the new parent class is the Python CDK ErrorHandler which does not have the max_time/max_retries, etc.