-
Notifications
You must be signed in to change notification settings - Fork 25
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 all 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 |
---|---|---|
|
@@ -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. | ||
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 | ||
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. 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={}), | ||
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={}, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In my opinion, the assignment of
config: Mapping[str, Any] = {}
is not necessary in this context. Instead, I suggest we instantiate theconfig
object within the__post_init__()
method and make this field optional, setting its default value toNone
. Please share your thoughts on this suggestion.