feat(manifest): add max_concurrent_async_job_count as an option in manifest#584
Conversation
📝 WalkthroughWalkthroughThe constructor of Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test
participant Source as ConcurrentDeclarativeSource
participant Factory as ModelToComponentFactory
participant Tracker as JobTracker
Test->>Source: Create with manifest (includes max_concurrent_async_job_count)
Source->>Factory: Instantiate (with max_concurrent_async_job_count)
Factory->>Tracker: Set job limit (max_concurrent_async_job_count)
Test->>Tracker: Acquire job intents (up to limit)
Test->>Tracker: Acquire job intent (exceed limit)
Tracker-->>Test: Raise ConcurrentJobLimitReached
Possibly related PRs
Suggested labels
Suggested reviewers
Would you like to consider adding a test for scenarios where ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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/concurrent_declarative_source.py (1)
92-92: Consider type validation for the async job count parameterThe parameter extraction looks good and safely handles missing keys. However, since
source_config.get()returnsAnyand the test shows manifest values can be strings, should we consider adding type conversion or validation here? For example, what happens if someone passes a non-numeric value? WDYT about adding a simple validation to ensure it's a positive integer when provided?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/sources/declarative/concurrent_declarative_source.py(1 hunks)unit_tests/sources/declarative/test_concurrent_declarative_source.py(2 hunks)
⏰ 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-hardcoded-records' (skip=false)
- GitHub Check: Check: 'source-shopify' (skip=false)
- GitHub Check: Pytest (Fast)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Analyze (python)
🔇 Additional comments (2)
unit_tests/sources/declarative/test_concurrent_declarative_source.py (2)
33-33: LGTM on the import additionClean import addition that's needed for the new test functionality.
1843-1859: Excellent test coverage for the new functionalityThis test is well-structured and comprehensive! I appreciate how it:
- Tests the end-to-end flow from manifest config to job tracker behavior
- Validates both the successful case (acquiring up to the limit) and the error case (exceeding the limit)
- Uses descriptive assertions to verify the internal state
The fact that you're setting the limit as a string in the manifest but expecting an integer in the job tracker suggests there's proper type conversion happening in the component factory - nice to see that working correctly.
Christo Grabowski (ChristoGrab)
left a comment
Christo Grabowski (ChristoGrab)
left a comment
There was a problem hiding this comment.
Aldo Gonzalez (aldogonzalez8)
deleted the
ac8/make-max_concurrent_async_job_count-available-in-manifest
branch
3 participants
Supporting max_concurrent_async_job_count (this was only working in the Connector Builder, but not during actual reads)
Resolves https://github.com/airbytehq/airbyte-internal-issues/issues/13236
Summary by CodeRabbit