fix: handle backoff_strategies in CompositeErrorHandler

[ChristoGrab]

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_strategies in the child ErrorHandler components, 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-None value as the strategy for both:

1. matches in response_filters
2. default retryable errors.

For example, using:

type: CompositeErrorHandler
error_handlers:
 - type: DefaultErrorHandler
   max_retries: 2
   backoff_strategies:
     - type: ConstantBackoffStrategy
       backoff_time_in_seconds: 10
   response_filters:
     - type: HttpResponseFilter
       action: RETRY
       http_codes:
         - 404
 - type: DefaultErrorHandler
   response_filters:
     - type: HttpResponseFilter
       action: RETRY
       http_codes:
         - 500
   max_retries: 3
   backoff_strategies:
     - type: ExponentialBackoffStrategy
       factor: 0.5

We would expect a 500 level response to utilize the Exponential strategy. However, the output seen is in fact:

Retrying. Sleeping for 10.0 seconds (rate_limiting.py:114)
Retrying. Sleeping for 10.0 seconds (rate_limiting.py:114)

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

  • Enhanced error handling with improved backoff strategy management in the Composite Error Handler.
  • Added ability to aggregate and prioritize backoff strategies across multiple error handlers.

• Tests

  • Added comprehensive test coverage for backoff strategy implementation.
  • Verified correct behavior of backoff strategies in HTTP request retry scenarios.

[maxi297]

Can we link the GitHub issue this change is related to?

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request introduces an enhancement to the CompositeErrorHandler class in the Airbyte CDK by adding a new backoff_strategies property method. This method aggregates backoff strategies from multiple child error handlers, allowing for a more flexible approach to handling retryable errors. The changes include the implementation of this method in the main class and the addition of corresponding unit tests to validate its behavior across various scenarios.

Changes

File	Change Summary
airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py	Added backoff_strategies property method to aggregate strategies from child error handlers
unit_tests/sources/declarative/requesters/error_handlers/test_composite_error_handler.py	Added tests for backoff_strategies method, including scenarios with multiple handlers and strategy prioritization
unit_tests/sources/declarative/requesters/test_http_requester.py	Added test to verify backoff strategy configuration from manifest

Assessment against linked issues

Objective	Addressed	Explanation
Configure Custom Backoff Strategy [#42928]	✅
Support Multiple Error Handlers with Backoff Strategies	✅

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 clarity of the new tests? They seem to cover a range of scenarios effectively! 😊

✨ Finishing Touches

• 📝 Generate Docstrings (Beta)

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

‼️ IMPORTANT
Auto-reply has been disabled for this repository in the CodeRabbit settings. The CodeRabbit bot will not respond to your replies unless it is explicitly tagged.

• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

• You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
• Please see the configuration documentation for more information.
• If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[coderabbitai]

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

📥 Commits

Reviewing files that changed from the base of the PR and between 34a978d and 168529d.

📒 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.

[maxi297]

LGTM! Can we create an issue to improve on backoff strategies and fix the issue when there are multiple error handlers in the composite error handler?

[ChristoGrab]

Created the issue to improve the handling of backoff strategies, linking here for additional visibility: https://github.com/airbytehq/airbyte-internal-issues/issues/11449

[coderabbitai]

Actionable comments posted: 0

🧹 Nitpick comments (2)

airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (2)

82-101: Great implementation! Consider adding a warning for multiple strategies?

The implementation looks solid and the docstring is very informative about the "first strategy wins" behavior. Since subsequent strategies are ignored, what do you think about adding a warning when multiple strategies are present? This could help users understand why their additional strategies aren't being used. wdyt?

Here's a potential enhancement:

     @property
     def backoff_strategies(self) -> Optional[List[BackoffStrategy]]:
         all_strategies = []
         for handler in self.error_handlers:
             if hasattr(handler, "backoff_strategies") and handler.backoff_strategies:
                 all_strategies.extend(handler.backoff_strategies)
+        if len(all_strategies) > 1:
+            import warnings
+            warnings.warn(
+                "Multiple backoff strategies detected. Only the first strategy will be used, subsequent strategies will be ignored.",
+                UserWarning
+            )
         return all_strategies if all_strategies else None

---

84-96: Enhance return type documentation?

The docstring is excellent in explaining the behavior! Would you consider adding a Returns section that explicitly documents the return type and its structure? Something like:

         """
         Combines backoff strategies from all child error handlers into a single flattened list.

         When used with HttpRequester, note the following behavior:
         - In HttpRequester.__post_init__, the entire list of backoff strategies is assigned to the error handler
         - However, the error handler's backoff_time() method only ever uses the first non-None strategy in the list
         - This means that if any backoff strategies are present, the first non-None strategy becomes the default
         - This applies to both user-defined response filters and errors from DEFAULT_ERROR_MAPPING
         - The list structure is not used to map different strategies to different error conditions
         - Therefore, subsequent strategies in the list will not be used

         Returns None if no handlers have strategies defined, which will result in HttpRequester using its default backoff strategy.
+
+        Returns:
+            Optional[List[BackoffStrategy]]: A list of backoff strategies from all child handlers,
+                                           or None if no strategies are defined.
         """

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 168529d and a55b33d.

📒 Files selected for processing (1)

• airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (2 hunks)

⏰ Context from checks skipped due to timeout of 90000ms (4)

• GitHub Check: Pytest (Fast)
• GitHub Check: Pytest (All, Python 3.11, Ubuntu)
• GitHub Check: Pytest (All, Python 3.10, Ubuntu)
• GitHub Check: Analyze (python)

🔇 Additional comments (1)

airbyte_cdk/sources/declarative/requesters/error_handlers/composite_error_handler.py (1)

11-11: LGTM! Clean import addition.

The BackoffStrategy import is well-placed and properly scoped for the new functionality.