-
Notifications
You must be signed in to change notification settings - Fork 101
Allow activity/workflow/nexus only workers to be registered on the same client/NS/TQ #1058
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
Allow activity/workflow/nexus only workers to be registered on the same client/NS/TQ #1058
Conversation
crates/sdk-core/src/worker/mod.rs
Outdated
| }); | ||
|
|
||
| // Determine worker capabilities based on configuration | ||
| let capabilities = temporalio_client::WorkerCapabilities { |
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.
I'm not 100% confident this is correct in how we detect workflow/nexus only workers
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.
Hrmm, I don't think langs tell Core today whether it will ever ask for workflow or Nexus work. This may be new (required) worker options. Never before has Core cared ahead of time which poll calls were going to be made.
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.
I actually kinda want to switch to that model anyway. The whole implicit polling thing complicates shutdown and switching to an up-front model would make some nasty code go away (and actually save some resources too).
The only complication would be if we ever expect allowing people to dynamically start polling for a certain kind when they weren't before... but... I'm not sure that's a scenario I think matters at all.
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.
The only complication would be if we ever expect allowing people to dynamically start polling for a certain kind when they weren't before... but... I'm not sure that's a scenario I think matters at all.
That could potentially be something we allow users to configure from server commands?
But would it be that be that complicated? I guess that would mean more code is kept in callbacks and more mutex/atomic variables to track state for worker commands and shutdown and such
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.
Yeah, good point. We can but I'm honestly not sure if we should. More knobs and complexity for unclear benefit. Though, maybe, it's kind of the same mechanism as worker "pause" which people have asked for.
I'm kinda worried about pausing/resuming polling (mostly for workflows) because shutdown is already really hard to get right and this is like extra-special shutdown.
But, regardless, I don't want you to deal with any of this right now. IMO it makes sense to add info about whether or not the worker polls for the 3 task kinds. For now, I'm perfectly fine with requiring lang to say up-front if the worker should start life polling for which task types. It can easily do that by just checking what is registered, like it does now to decide if it should make poll calls at all. If it calls poll on something it didn't say should be turned on, just immediately return PollError::Shutdown
All the other stuff we can address later
2b33733 to
3e0b754
Compare
| { | ||
| for existing_worker_info in existing_workers { | ||
| if existing_worker_info.build_id.as_ref() == build_id.as_ref() | ||
| && task_types.overlaps_with(&existing_worker_info.task_types) |
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.
My only concern here would be if langs do not start setting this value right away and therefore it'd look like it overlaps when it doesn't but I think the removal of the default from #1059 ensures that won't happen, so we're good.
What was changed
Make
worker_registryaware ofWorkerTaskType, to allow multiple workers without overlapping types to be registered on the same client/NS/TQWhy?
We want to support this to minimize the breakage for users, since previously this scenario did not return an error. This is likely primarily used in tests or maybe code organization.
Checklist
Closes
How was this tested:
Added new tests
Note
Enables registering workflow/activity/nexus-only workers on the same client/namespace/task queue when their task types don’t overlap, updates slot selection to use task types, and adds validations/tests.
crates/client/src/worker_registry):RegisteredWorkerInfoincludingbuild_idandWorkerTaskTypes.namespace/task_queueif theirtask_typesdon’t overlap (samebuild_idallowed if non-overlapping); reject empty capabilities.enable_workflows.ClientWorkertrait withworker_task_types().crates/common/src/worker.rs):WorkerTaskTypes::overlaps_withhelper; validateWorkerConfig.task_typesand related invariants.crates/sdk-core/src/worker/mod.rs):config.task_typesinto client worker registration; implementworker_task_types()inClientWorkerRegistrator.task_types(workflows/activities/nexus) where applicable.crates/client/src/raw.rs):WorkerTaskTypeswhen registering workers for eager start path.Written by Cursor Bugbot for commit 3e0b754. This will update automatically on new commits. Configure here.