-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Concurrency limit UX enhancements: add strict
mode
#15297
Conversation
CodSpeed Performance ReportMerging #15297 will not alter performanceComparing Summary
|
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.
This all looks right to me! 👍
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.
lgtm!
async def test_concurrency_can_be_used_within_a_flow_strictly(): | ||
@task | ||
async def resource_heavy(): | ||
async with concurrency("santa-clause", occupy=1, strict=True): |
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.
nice one heh
This PR deprecates
create_if_missing
which causes an ill-posed problem: creating a limit implicitly has no obvious default limit value.Instead, the behavior we want is:
None
or from a limit that is "inactive" and therefore - at this instant - might as well not exist, i.e., it allows execution without creating any new object in the backend (and we log aWARNING
to alert users)strict: bool
that specifies whether to raise an error if the limit name doesn't exist in the backend; this defaults toFalse
but can be set toTrue
. Setting this toTrue
is satisfying the need of "I really want to make sure I have full control over the circumstance in which this runs" and if the limit doesn't exist, we don't yet know those circumstances and so we prevent execution through a raised exceptionI believe I need to revisit the docs for these utilities, but in the meantime wanted to get some eyes on the code.