-
Notifications
You must be signed in to change notification settings - Fork 32
feat: AsyncRetriever: Enable configurability of max concurrent job count #391
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
f99970b
463ad81
ed6bac5
0a53391
a73ea65
f42fdab
ab227b2
5215f3c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,11 @@ | |
| import logging | ||
| import threading | ||
| import uuid | ||
| from typing import Set | ||
| from typing import Any, Mapping, Set, Union | ||
|
|
||
| import json | ||
| from airbyte_cdk.logger import lazy_log | ||
| from airbyte_cdk.sources.declarative.interpolation import InterpolatedString | ||
|
|
||
| LOGGER = logging.getLogger("airbyte") | ||
|
|
||
|
|
@@ -15,7 +17,13 @@ class ConcurrentJobLimitReached(Exception): | |
|
|
||
|
|
||
| class JobTracker: | ||
| def __init__(self, limit: int): | ||
| def __init__(self, limit: Union[int, str], config: Mapping[str, Any] = {}): | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion, the assignment of |
||
| if isinstance(limit, str): | ||
| limit = int(InterpolatedString(limit, parameters={}).eval(config=config)) | ||
|
|
||
| if limit < 1: | ||
| raise ValueError(f"Invalid max concurrent jobs limit: {limit}. Minimum value is 1.") | ||
pnilan marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| self._jobs: Set[str] = set() | ||
| self._limit = limit | ||
| self._lock = threading.Lock() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3376,6 +3376,16 @@ definitions: | |
| - "$ref": "#/definitions/IterableDecoder" | ||
| - "$ref": "#/definitions/XmlDecoder" | ||
| - "$ref": "#/definitions/ZipfileDecoder" | ||
| max_concurrent_jobs: | ||
| title: Maximum Conccurent Job Count | ||
| description: Maximum number of concurrent jobs to run. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: it'd be useful to explain what those jobs do — are those parsing already downloaded archives, or are those threads that request multiple reports in parallel? etc |
||
| anyOf: | ||
| - type: integer | ||
| - type: string | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced you really need this in config, and therefore you need strings here. But, up to you.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I think you're right. Looks like Salesforce defines a static max across all accounts. |
||
| default: 1 | ||
| examples: | ||
| - 2 | ||
| - "{{ config['max_concurrent_jobs'] }}" | ||
| $parameters: | ||
| type: object | ||
| additionalProperties: true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -26,7 +26,7 @@ def test_stream_slices_with_single_partition_router(): | |
| job_orchestrator_factory=lambda stream_slices: AsyncJobOrchestrator( | ||
| MockAsyncJobRepository(), | ||
| stream_slices, | ||
| JobTracker(_NO_LIMIT), | ||
| JobTracker(_NO_LIMIT, config={}), | ||
| NoopMessageRepository(), | ||
| ), | ||
| config={}, | ||
|
|
@@ -58,7 +58,7 @@ def test_stream_slices_with_parent_slicer(): | |
| job_orchestrator_factory=lambda stream_slices: AsyncJobOrchestrator( | ||
| MockAsyncJobRepository(), | ||
| stream_slices, | ||
| JobTracker(_NO_LIMIT), | ||
| JobTracker(_NO_LIMIT, config={}), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Seems like JobTracker already has config={} default in the initializer? Could avoid changes here I think? |
||
| NoopMessageRepository(), | ||
| ), | ||
| config={}, | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.