feat: enable async job configurability via config#435
feat: enable async job configurability via config#435Patrick Nilan (pnilan) merged 4 commits intomainfrom
config#435Conversation
📝 WalkthroughWalkthroughThis pull request introduces modifications to the job tracking and component schema logic. In the Changes
Sequence Diagram(s)sequenceDiagram
participant JT as JobTracker
participant IS as InterpolatedString
participant Log as Logger
JT->>JT: __post_init__() invocation
alt limit is string
JT->>IS: Evaluate limit
alt Evaluation successful
IS-->>JT: Return numeric limit
JT->>JT: Set _limit (ensure >= 1)
else Evaluation fails
IS-->>JT: Throw exception
JT->>Log: Log warning message
JT->>JT: Set _limit to 1
end
else limit is an integer
JT->>JT: Validate and assign _limit (ensure >= 1)
end
Possibly related PRs
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: 2
🧹 Nitpick comments (5)
unit_tests/sources/declarative/async_job/test_job_tracker.py (1)
62-67: Test name matches behavior, but consider making it more specific.This test correctly verifies that when interpolation fails, the limit is set to 1. The test name could be more specific about what makes interpolation fail (in this case, a non-integer value). wdyt?
-def test_given_interpolated_limit_and_empty_config_when_init_then_set_to_1(): +def test_given_non_integer_interpolated_value_when_init_then_set_to_1(): tracker = JobTracker( "{{ config['max_concurrent_async_job_count'] }}", {"max_concurrent_async_job_count": "hello"}, ) assert tracker._limit == 1airbyte_cdk/sources/declarative/async_job/job_tracker.py (1)
24-41: Well-implemented string interpolation with appropriate error handling.The implementation for handling string limits and interpolation looks good. You're gracefully handling errors and providing clear warning messages. Small suggestion: consider adding a return type hint for
__post_init__for completeness, wdyt?- def __post_init__(self): + def __post_init__(self) -> None: self._jobs: Set[str] = set() self._lock = threading.Lock() if isinstance(self.limit, str): try: self.limit = int( InterpolatedString(self.limit, parameters={}).eval(config=self.config) ) except Exception as e: LOGGER.warning( f"Error interpolating max job count: {self.limit}. Setting to 1. {e}" ) self.limit = 1 if self.limit < 1: LOGGER.warning( f"The `max_concurrent_async_job_count` property is less than 1: {self.limit}. Setting to 1. Please update the source manifest to set a valid value." ) self._limit = self.limit if self.limit >= 1 else 1airbyte_cdk/sources/declarative/declarative_component_schema.yaml (3)
50-55: Enhanced Flexibility formax_concurrent_async_job_count
You've updated the property to accept bothintegerandstringtypes and added examples (including an interpolated configuration value), which nicely enables dynamic async job configurability. Would you consider adding some guidance or constraints (such as expected ranges or value formats) to further help users understand acceptable inputs? wdyt?
2902-2902: Consistent Default Formatting forlazy_read_pointer
The default value has been reformatted from[ ]to[], which improves consistency with our YAML style guidelines. Was this reformatting aimed at resolving any noted discrepancies in our defaults? wdyt?
3207-3207: Clean Enum Formatting inStateDelegatingStream
The enum formatting has been tightened to remove extra spaces (now[StateDelegatingStream]), which enhances readability and adheres to best practices. Do you think we should perform a similar review on other enum definitions to ensure uniform styling? wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
airbyte_cdk/sources/declarative/async_job/job_tracker.py(2 hunks)airbyte_cdk/sources/declarative/declarative_component_schema.yaml(3 hunks)airbyte_cdk/sources/declarative/models/declarative_component_schema.py(2 hunks)unit_tests/sources/declarative/async_job/test_job_tracker.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
unit_tests/sources/declarative/async_job/test_job_tracker.py (1)
airbyte_cdk/sources/declarative/async_job/job_tracker.py (1) (1)
JobTracker(20-95)
🪛 GitHub Actions: Linters
airbyte_cdk/sources/declarative/async_job/job_tracker.py
[error] 3-3: Ruff: Import block is un-sorted or un-formatted. Organize imports.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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)
🔇 Additional comments (3)
airbyte_cdk/sources/declarative/models/declarative_component_schema.py (2)
1893-1897: LGTM! Good addition of string support for max_concurrent_async_job_count.The change to support both integer and string types for the
max_concurrent_async_job_countfield along with the examples is well-implemented and aligns with the changes in theJobTrackerclass.
1926-1930: LGTM! Consistent implementation for DeclarativeSource2.The changes are consistently applied to both
DeclarativeSource1andDeclarativeSource2classes, which is good for maintaining code consistency.airbyte_cdk/sources/declarative/async_job/job_tracker.py (1)
19-23: LGTM! Good use of dataclass for simplifying the code.Converting
JobTrackerto a dataclass and adding type hints for the attributes is a great improvement. The default_factory for the config parameter ensures backward compatibility.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
unit_tests/sources/declarative/async_job/test_job_tracker.py (1)
62-67: Great error handling test case!This test correctly verifies the fallback behavior when an interpolated string doesn't evaluate to a valid integer. The implementation will set
_limitto 1 in this case, which is what you're testing.As a suggestion - would it be worth adding another test case where the config is completely empty? Something like:
def test_given_interpolated_limit_and_missing_config_when_init_then_set_to_1(): tracker = JobTracker("{{ config['max_concurrent_async_job_count'] }}", {}) assert tracker._limit == 1Just to cover that scenario too. wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/async_job/test_job_tracker.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
unit_tests/sources/declarative/async_job/test_job_tracker.py (1)
airbyte_cdk/sources/declarative/async_job/job_tracker.py (1) (1)
JobTracker(20-95)
🪛 GitHub Actions: Linters
unit_tests/sources/declarative/async_job/test_job_tracker.py
[error] 1-1: Ruff formatting check failed. 1 file would be reformatted. Run 'ruff format' to fix code style issues in this file.
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
unit_tests/sources/declarative/async_job/test_job_tracker.py (2)
66-71: Nice error handling test!This test properly validates the fallback behavior when interpolation fails to resolve to a valid integer. It aligns perfectly with the
__post_init__implementation that setslimitto 1 when an exception occurs during interpolation.One suggestion: would it be valuable to add another test case where the interpolated string resolves to a negative value? This would verify that both direct validation and interpolation fallback to 1 when the value is invalid. wdyt?
50-63: Verify type annotationsThe parameters don't have type annotations. Do you think adding type hints like
(limit: str, config: dict, expected_limit: int)would enhance code readability? This would align with the typing convention used in the existing test at line 45.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
unit_tests/sources/declarative/async_job/test_job_tracker.py(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
unit_tests/sources/declarative/async_job/test_job_tracker.py (1)
airbyte_cdk/sources/declarative/async_job/job_tracker.py (1) (1)
JobTracker(20-95)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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 (Fast)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Analyze (python)
🔇 Additional comments (1)
unit_tests/sources/declarative/async_job/test_job_tracker.py (1)
50-63: Good work on the updated parameterized test!The test now correctly validates that string limits properly interpolate using config values, with clear parameters and assertions. This matches the implementation in
job_tracker.pywhere string values are interpolated and converted to integers.
Patrick Nilan (pnilan)
deleted the
pnilan/async/enable-job-limit-configurability
branch
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests