fix(manifest server): default concurrency settings#792
Conversation
👋 Greetings, Airbyte Team Member!Here are some helpful tips and reminders for your convenience. Testing This CDK VersionYou can test this version of the CDK using the following: # Run the CLI from this branch:
uvx 'git+https://github.com/airbytehq/airbyte-python-cdk.git@maxi297/connector_builder_fix_concurrency#egg=airbyte-python-cdk[dev]' --help
# Update a connector to use the CDK from this branch ref:
cd airbyte-integrations/connectors/source-example
poe use-cdk-branch maxi297/connector_builder_fix_concurrencyHelpful ResourcesPR Slash CommandsAirbyte Maintainers can execute the following slash commands on your PR:
|
📝 WalkthroughWalkthroughRemoved automatic injection of Changes
Sequence Diagram(s)sequenceDiagram
participant Router as Manifest Router (test_read)
participant Request as Request.Manifest
participant SafeBuild as safe_build_source / build_source
Router->>Request: deep-copy manifest
Router->>Router: ensure concurrency_level.default_concurrency = 1
Router->>SafeBuild: call safe_build_source(augmented manifest, config, catalog, state)
SafeBuild->>SafeBuild: build_source uses provided manifest (no injection)
SafeBuild-->>Router: return built source / error
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Should we add a unit test that verifies behavior when a manifest lacks Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
🔇 Additional comments (1)
Comment |
PyTest Results (Fast)3 796 tests - 1 3 784 ✅ - 1 6m 35s ⏱️ +11s Results for commit 1991c07. ± Comparison against base commit 8158f0d. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
PyTest Results (Full)3 799 tests - 1 3 787 ✅ - 1 11m 19s ⏱️ +5s Results for commit 1991c07. ± Comparison against base commit 8158f0d. This pull request removes 1 test.♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
airbyte_cdk/manifest_server/routers/manifest.py (1)
91-100: Nice fix for the deadlock issue!The logic correctly enforces
default_concurrency = 1for test_read to prevent the partition generator from overwhelming worker threads, while preserving ordering for the builder message responses. The deep copy and conditional handling look solid.One optional thought: if this pattern might be needed elsewhere in the future, would extracting this into a helper function like
enforce_single_threaded_concurrency(manifest_dict)make it more reusable? Just a suggestion though—totally fine to keep it inline for now, wdyt?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
airbyte_cdk/manifest_server/command_processor/utils.py(0 hunks)airbyte_cdk/manifest_server/routers/manifest.py(2 hunks)
💤 Files with no reviewable changes (1)
- airbyte_cdk/manifest_server/command_processor/utils.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Check: destination-motherduck
- GitHub Check: Check: source-shopify
- GitHub Check: Check: source-hardcoded-records
- GitHub Check: Check: source-pokeapi
- GitHub Check: Check: source-intercom
- GitHub Check: Pytest (All, Python 3.13, Ubuntu)
- GitHub Check: Pytest (All, Python 3.11, Ubuntu)
- GitHub Check: Pytest (All, Python 3.10, Ubuntu)
- GitHub Check: Pytest (All, Python 3.12, Ubuntu)
- GitHub Check: SDM Docker Image Build
- GitHub Check: Manifest Server Docker Image Build
- GitHub Check: Pytest (Fast)
🔇 Additional comments (2)
airbyte_cdk/manifest_server/routers/manifest.py (2)
1-1: LGTM!The
copyimport is needed for the deep copy operation below and is placed appropriately with the other imports.
102-110: LGTM!The call to
safe_build_sourcenow correctly uses the modifiedmanifestwith the enforced concurrency settings. This ensures the single-threaded behavior is applied during source building.
|
/autofix
|
3 participants
Users are entering a deadlock state where the partition generator threads are filling the queue of partitions to process without having a thread to relieve the queue.
See: https://airbytehq-team.slack.com/archives/C09B97YAB2B/p1760532962689779
Summary by CodeRabbit
Bug Fixes
Tests