fix: (CDK) (AsyncRetriever) - Add polling_timeout optional field
#428
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors and extends the AsyncRetriever component by adding an optional polling_timeout field and making related adjustments across the CDK. Key changes include:
- Updating the Pydantic model and YAML schema for AsyncRetriever with a new polling_timeout property.
- Removing deprecated warnings from the AsyncRetriever class.
- Adjusting the HTTPJobRepository and model-to-component factory to incorporate job_timeout based on polling_timeout.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| airbyte_cdk/sources/declarative/models/declarative_component_schema.py | Updated AddFields description and added polling_timeout to AsyncRetriever. |
| airbyte_cdk/sources/declarative/declarative_component_schema.yaml | Revised AsyncRetriever description and introduced polling_timeout property. |
| airbyte_cdk/sources/declarative/requesters/http_job_repository.py | Updated job_timeout comment and enhanced the create job message to include job_timeout. |
| airbyte_cdk/sources/declarative/retrievers/async_retriever.py | Removed deprecated decorator and ExperimentalClassWarning. |
| airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py | Added _get_job_timeout helper and passed job_timeout to AsyncHttpJobRepository. |
Comments suppressed due to low confidence (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py:2942
- [nitpick] Consider renaming variables like 'test_read_timeout' and 'default_sync_timeout' to names that more clearly indicate their purpose in distinguishing connector builder behavior from default sync timeouts.
def _get_job_timeout() -> datetime.timedelta:
📝 WalkthroughWalkthroughThis pull request refines the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User Config
participant M as ModelToComponentFactory
participant J as AsyncHttpJobRepository
U->>M: Provide polling_timeout configuration
M->>M: Call _get_job_timeout() to calculate timeout
M->>J: Instantiate AsyncHttpJobRepository with calculated job_timeout
J->>J: Log job creation with timeout information
Possibly related PRs
Suggested labels
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
🪧 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 (1)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
3384-3391: Nice addition of thepolling_timeoutproperty!This new field provides a clear way to handle timeouts for async jobs, which is an important feature for reliability. The support for both integer and string types with interpolation context offers good flexibility for configuration-based values.
Have you considered adding an example value? Something like:
examples: - 30 - "{{ config['timeout_minutes'] }}"Wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/requesters/http_job_repository.py(2 hunks)airbyte_cdk/sources/declarative/retrievers/async_retriever.py(0 hunks)
💤 Files with no reviewable changes (1)
- airbyte_cdk/sources/declarative/retrievers/async_retriever.py
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Check: 'source-amplitude' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
🔇 Additional comments (9)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml (1)
3336-3336: Description update looks good!The removal of the experimental notice suggests this component is now considered stable. This aligns well with removing the deprecated warnings mentioned in the PR description. Good cleanup!
airbyte_cdk/sources/declarative/requesters/http_job_repository.py (3)
48-49: Good addition of the job_timeout propertyThe attribute and its documentation clearly explain that it's derived from the
polling_timeoutparameter. The typing asOptional[timedelta]is appropriate since this might not always be specified.
136-136: Nice dynamic description!Including the timeout value in the job creation log message will make debugging timeout issues much easier. This change provides important context for both users and developers.
166-166: Using the timeout parameter successfullyI see that you're correctly passing the
job_timeoutto theAsyncJobconstructor. This ensures the timeout is properly propagated to the job instance.Just curious - is there any additional handling of the timeout in the
AsyncJobclass to make the timeout effective? If so, that might be worth mentioning in the PR description for completeness.airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
1470-1477: Nice typo fix in the description!You cleaned up the extraneous comma at the end of the sentence. Small details like this improve documentation quality.
2357-2360: The newpolling_timeoutfield looks good!This addition will allow users to specify a custom timeout for asynchronous jobs, which aligns with the PR objectives to fix issues #11209 and #11923. The field supports both integer and string types for flexibility, with a clear description.
Just to confirm - are there any default timeout values that would be good to document in the description? It might help users understand the expected behavior when
Noneis provided, wdyt?airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (3)
510-510: Import cleanup looks good!Removing the unused import of
ConnectionDefinitionhelps keep the codebase clean.
2942-2962: New timeout handling function looks good!This function handles both string and numeric timeout values elegantly, with sensible defaults for different contexts (15 min for test reads, 60 min for production syncs).
I like how you're using the
InterpolatedStringfor evaluation - this allows for flexible configuration via string expressions.
3067-3067: Good integration of the timeout parameter!The
job_timeoutparameter is now properly passed toAsyncHttpJobRepository, completing the implementation of the configurable timeout feature.
airbyte_cdk/sources/declarative/models/declarative_component_schema.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (1)
2942-2962: Nice implementation of timeout handling with defaults!The new
_get_job_timeout()method provides a clear way to determine job timeouts with sensible defaults. I like how it handles both user-defined timeouts and fallback values differently for test reads vs normal sync operations.One thought - have you considered adding a docstring to explain the method's purpose and parameter handling? It might help future developers understand the logic more quickly, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/declarative_component_schema.yaml(2 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py(4 hunks)airbyte_cdk/sources/declarative/requesters/http_job_repository.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- airbyte_cdk/sources/declarative/requesters/http_job_repository.py
- airbyte_cdk/sources/declarative/declarative_component_schema.yaml
- airbyte_cdk/sources/declarative/models/declarative_component_schema.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Check: 'source-pokeapi' (skip=false)
- GitHub Check: Check: 'source-amplitude' (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)
airbyte_cdk/sources/declarative/parsers/model_to_component_factory.py (2)
510-510: Clean up: Import simplified to Config onlyThe removal of the unused
ConnectionDefinitionimport helps keep the imports clean and focused. Good improvement!
3067-3067: Well integrated job_timeout parameterGood job passing the timeout from the newly created method to the
AsyncHttpJobRepository. This nicely completes the implementation of the polling timeout feature.
What
Resolving:
How
Summary of Changes
declarative_component_schema.yaml:experimental noticefrom theAsyncRetrieverdescription.AddFieldsclass description.polling_timeoutproperty toAsyncRetrieverwith a description and support for integer and string types.model_to_component_factory.py:ConnectionDefinitionimport._get_job_timeoutmethod to determine job timeout.http_job_repository.py:async_retriever.py:User Impact
No impact is expected. This is not a breaking change.
Summary by CodeRabbit