Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[airbyte-cdk] Updates Low Code CDK ErrorHandlers and BackoffStrategies to align with Python CDK Interfaces #38743
Changes from 43 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
There are no files selected for viewing
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.
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.
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.
The thought behind the current indentation levels/response hierarchies is:
a. If response is SUCCESS, return success resolution
b. If response is RETRY or IGNORE, return resolution
c. If response is FAIL, iterate to next error handler to see if there is a "better" specified action for the given response
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 comment
The 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?