Expose poller autoscaling options #830
Conversation
Sushisource
force-pushed
the
worker-deployment-versioning
branch
from
0a81383 to
424c2e7
Compare
temporalio/worker/_worker.py
Outdated
| @@ -48,6 +49,48 @@ | |||
| logger = logging.getLogger(__name__) | |||
|
|
|||
|
|
|||
| @dataclass | |||
There was a problem hiding this comment.
Would recommend these be set to frozen, especially this one with how it's used as a default in a parameter (Python parameter defaults are a bit strange in that that expression is evaluated once and reused for every default).
temporalio/worker/_worker.py
Outdated
| workflow_task_poller_behavior: Specify the behavior of workflow task polling | ||
| activity_task_poller_behavior: Specify the behavior of activity task polling |
There was a problem hiding this comment.
I have a request here and also for tuner since it has tripped me up since we removed the default parameters for the max_concurrent fields too. Can you say in the doc strings what the defaults are since they are no longer obvious scalars? Would request this for these two values and for tuner.
| workflow_task_poller_behavior: PollerBehavior = PollerBehaviorSimpleMaximum( | ||
| maximum=5 | ||
| ), |
There was a problem hiding this comment.
May be clearer if there is a default ClassVar on the dataclass that represents the default, e.g. PollerBehaviorSimpleMaximum.default instead of inline code, but not required
In fact, looking at params here, if you fix the issue where you're accidentally ignoring the max_concurrent_workflow_task_polls and max_concurrent_activity_task_polls now, you're going to need to know whether one of the other is set, so you may need a arg-unset sentinel type of value here instead of a valuable default.
There was a problem hiding this comment.
Ah yeah I meant to go back and add comments saying max_ will override if set and implement that behavior
| @@ -156,6 +205,8 @@ def __init__( | |||
| max_concurrent_workflow_task_polls: Maximum number of concurrent | |||
| poll workflow task requests we will perform at a time on this | |||
| worker's task queue. | |||
|
|
|||
| WARNING: Deprecated, use ``workflow_task_poller_behavior`` instead | |||
There was a problem hiding this comment.
I can't find where this or the activity one is used anymore
| @@ -931,6 +932,29 @@ def state(self): | |||
| # ) | |||
|
|
|||
|
|
|||
| async def test_can_run_autoscaling_polling_worker( | |||
There was a problem hiding this comment.
Is there any way we can assert that Core is at least working with what was passed? Maybe via metrics or something?
There was a problem hiding this comment.
Did it with metrics on startup.
Fun fact: Don't forget that the union discrimination across the pyo3 boundary relies on the variants not having same-named fields. That tripped me up for a while trying to figure out why the config was set properly but not read by core properly, so, test was worth it.
There was a problem hiding this comment.
union discrimination across the pyo3 boundary
I am not sure I have ever used unions in PyO3. When it comes to lang boundaries, I prefer sets of mutually exclusive fields with runtime checks as I'm afraid of the translation layers for unions or other advanced constructs.
Sushisource
force-pushed
the
worker-deployment-versioning
branch
6 times, most recently
from
3d1c940 to
707473a
Compare
Sushisource
force-pushed
the
expose-poller-automation
branch
2 times, most recently
from
cc99a8d to
30e00ae
Compare
Sushisource
force-pushed
the
expose-poller-automation
branch
2 times, most recently
from
37c8155 to
76d5e6d
Compare
Sushisource
force-pushed
the
expose-poller-automation
branch
from
76d5e6d to
3b133f2
Compare
Requires #821 to merge first
What was changed
Expose options to opt-in to new poller autoscaling behavior
Why?
Checklist
Closes
How was this tested:
Added simple test. More meaningful tests are in Core, we just need to make sure option is passed properly here.
Any docs updates needed?