fix: handle backoff_strategies in CompositeErrorHandler #225
Conversation
| ), | ||
| ], | ||
| ) | ||
| def test_composite_error_handler_backoff_strategies( |
There was a problem hiding this comment.
This test ensures that the right strategies are returned which is good. It think there is still a test missing to make sure that when we bootstrap from a manifest, the backoff strategies are actually passed/used by the HttpClient. WDYT?
There was a problem hiding this comment.
Yup, that makes sense to me as a closer indicator of integration with the declared manifest, will add!
📝 WalkthroughWalkthroughThe pull request introduces an enhancement to the Changes
Assessment against linked issues
The changes directly address the issue of configuring error handlers for backoff strategies, providing more flexibility in handling retryable errors. The implementation allows for custom backoff strategies to be properly configured and used, which was not working in previous versions. Wdyt about the approach? The new implementation seems to provide a more robust way of handling backoff strategies across multiple error handlers. 🤔 Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)
82-96: LGTM! The backoff_strategies implementation looks solid.The implementation correctly aggregates strategies from all handlers and handles edge cases well. The docstring is particularly helpful in explaining the current behavior and limitations.
Just a thought - would it make sense to add a debug log when no strategies are found, to help with troubleshooting? wdyt?
unit_tests/sources/declarative/requesters/test_http_requester.py (1)
908-936: Integration test looks good! Addresses the previous review feedback.The test effectively validates that backoff strategies from the manifest are respected during HTTP requests. Nice job following up on the previous review comment about manifest integration!
One suggestion - would it be helpful to also verify the actual backoff time used? For example, checking that the 0.1s constant backoff was applied? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py(2 hunks)unit_tests/sources/declarative/requesters/error_handlers/test_composite_error_handler.py(2 hunks)unit_tests/sources/declarative/requesters/test_http_requester.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-the-guardian-api' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Check: 'source-hardcoded-records' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
unit_tests/sources/declarative/requesters/error_handlers/test_composite_error_handler.py (2)
280-314: Great test coverage! The parameterized tests look comprehensive.The test cases effectively cover all important scenarios including empty, single, and multiple handler configurations.
317-351: The strategy prioritization test is well thought out.Good job testing that the first handler's strategy is respected even when the second handler's response filter is triggered. This is crucial for validating the behavior described in the docstring.
What
Although users can define custom backoff logic in the manifest, the CompositeErrorHandler component does not currently access the backoff_strategies, meaning it always defaults to the default exponential behavior. By accessing the
backoff_strategiesin the childErrorHandlercomponents, we can handle user-specified strategies.Resolves: airbytehq/airbyte#42928
Cautionary Note
My understanding of the ErrorHandler flow is that it is not currently configured to respect multiple backoff strategies for a single stream, and will always use the first non-
Nonevalue as the strategy for both:response_filtersFor example, using:
We would expect a
500level response to utilize the Exponential strategy. However, the output seen is in fact:This is a function of the errorhandler logic flow and is a known behavior, distinct from the bug this PR tries to address.
Summary by CodeRabbit
New Features
Tests