-
Notifications
You must be signed in to change notification settings - Fork 101
💥 Add explicit Worker configuration for type (workflow/activity/nexus) #1059
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
7a7eb99
07538f4
1c94eeb
85ca74b
9ceb6d8
98b0c9c
4044f57
5deef19
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 |
|---|---|---|
|
|
@@ -16,6 +16,59 @@ use std::{ | |
| time::Duration, | ||
| }; | ||
|
|
||
| /// Specifies which task types a worker will poll for. | ||
| /// | ||
| /// Workers can be configured to handle any combination of workflows, activities, and nexus operations. | ||
| #[derive(Clone, Copy, Debug, PartialEq, Eq, Hash)] | ||
| pub struct WorkerTaskTypes { | ||
| pub enable_workflows: bool, | ||
| pub enable_activities: bool, | ||
| pub enable_nexus: bool, | ||
| } | ||
|
|
||
| impl WorkerTaskTypes { | ||
| /// Check if no task types are enabled | ||
| pub fn is_empty(&self) -> bool { | ||
| !self.enable_workflows && !self.enable_activities && !self.enable_nexus | ||
| } | ||
|
|
||
| /// Create a config with all task types enabled | ||
| pub fn all() -> WorkerTaskTypes { | ||
| WorkerTaskTypes { | ||
| enable_workflows: true, | ||
| enable_activities: true, | ||
| enable_nexus: true, | ||
| } | ||
| } | ||
|
|
||
| /// Create a config with only workflow tasks enabled | ||
| pub fn workflow_only() -> WorkerTaskTypes { | ||
| WorkerTaskTypes { | ||
| enable_workflows: true, | ||
| enable_activities: false, | ||
| enable_nexus: false, | ||
| } | ||
| } | ||
|
|
||
| /// Create a config with only activity tasks enabled | ||
| pub fn activity_only() -> WorkerTaskTypes { | ||
| WorkerTaskTypes { | ||
| enable_workflows: false, | ||
| enable_activities: true, | ||
| enable_nexus: false, | ||
| } | ||
| } | ||
|
|
||
| /// Create a config with only nexus tasks enabled | ||
| pub fn nexus_only() -> WorkerTaskTypes { | ||
| WorkerTaskTypes { | ||
| enable_workflows: false, | ||
| enable_activities: false, | ||
| enable_nexus: true, | ||
| } | ||
| } | ||
| } | ||
|
|
||
| /// Defines per-worker configuration options | ||
| #[derive(Clone, derive_builder::Builder)] | ||
| #[builder(setter(into), build_fn(validate = "Self::validate"))] | ||
|
|
@@ -64,10 +117,10 @@ pub struct WorkerConfig { | |
| /// worker's task queue | ||
| #[builder(default = "PollerBehavior::SimpleMaximum(5)")] | ||
| pub nexus_task_poller_behavior: PollerBehavior, | ||
| /// If set to true this worker will only handle workflow tasks and local activities, it will not | ||
| /// poll for activity tasks. | ||
| #[builder(default = "false")] | ||
| pub no_remote_activities: bool, | ||
| /// Specifies which task types this worker will poll for. | ||
| /// | ||
| /// Note: At least one task type must be specified or the worker will fail validation. | ||
| pub task_types: WorkerTaskTypes, | ||
|
Member
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. Is there somewhere in the client code that has to be altered to properly account for this as part of the worker dedupe check?
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. Planning on keeping that PR separate, #1059, since that feature introduces different behavior, whereas this PR introduces a new feature, but maintains existing behavior, and to make review easier |
||
| /// How long a workflow task is allowed to sit on the sticky queue before it is timed out | ||
| /// and moved to the non-sticky queue where it may be picked up by any worker. | ||
| #[builder(default = "Duration::from_secs(10)")] | ||
|
|
@@ -218,6 +271,15 @@ impl WorkerConfigBuilder { | |
| } | ||
|
|
||
| fn validate(&self) -> Result<(), String> { | ||
| let task_types = self | ||
| .task_types | ||
| .as_ref() | ||
| .cloned() | ||
| .unwrap_or_else(WorkerTaskTypes::all); | ||
| if task_types.is_empty() { | ||
| return Err("At least one task type must be enabled in `task_types`".to_owned()); | ||
| } | ||
|
|
||
| if let Some(b) = self.workflow_task_poller_behavior.as_ref() { | ||
| b.validate()? | ||
| } | ||
|
|
@@ -249,6 +311,17 @@ impl WorkerConfigBuilder { | |
| return Err("`max_outstanding_nexus_tasks` must be > 0".to_owned()); | ||
| } | ||
|
|
||
| // Validate workflow cache is consistent with task_types | ||
| if !task_types.enable_workflows | ||
| && let Some(cache) = self.max_cached_workflows.as_ref() | ||
| && *cache > 0 | ||
| { | ||
| return Err( | ||
| "Cannot have `max_cached_workflows` > 0 when workflows are not enabled in `task_types`" | ||
| .to_owned(), | ||
| ); | ||
| } | ||
|
|
||
| if let Some(cache) = self.max_cached_workflows.as_ref() | ||
| && *cache > 0 | ||
| { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,121 @@ | ||
| use temporalio_common::worker::{WorkerConfigBuilder, WorkerTaskTypes, WorkerVersioningStrategy}; | ||
|
|
||
| fn default_versioning_strategy() -> WorkerVersioningStrategy { | ||
| WorkerVersioningStrategy::None { | ||
| build_id: String::new(), | ||
| } | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_default_configuration_polls_all_types() { | ||
| let config = WorkerConfigBuilder::default() | ||
| .namespace("default") | ||
| .task_queue("test-queue") | ||
| .versioning_strategy(default_versioning_strategy()) | ||
| .task_types(WorkerTaskTypes::all()) | ||
| .build() | ||
| .expect("Failed to build default config"); | ||
|
|
||
| let effective = &config.task_types; | ||
| assert!( | ||
| effective.enable_workflows, | ||
| "Should poll workflows by default" | ||
| ); | ||
| assert!( | ||
| effective.enable_activities, | ||
| "Should poll activities by default" | ||
| ); | ||
| assert!(effective.enable_nexus, "Should poll nexus by default"); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_empty_task_types_fails_validation() { | ||
| let result = WorkerConfigBuilder::default() | ||
| .namespace("default") | ||
| .task_queue("test-queue") | ||
| .versioning_strategy(default_versioning_strategy()) | ||
| .task_types(WorkerTaskTypes { | ||
| enable_workflows: false, | ||
| enable_activities: false, | ||
| enable_nexus: false, | ||
| }) | ||
| .build(); | ||
|
|
||
| assert!(result.is_err(), "Empty task_types should fail validation"); | ||
| let err = result.err().unwrap().to_string(); | ||
| assert!( | ||
| err.contains("At least one task type"), | ||
| "Error should mention task types: {err}", | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_workflow_cache_without_workflows_fails() { | ||
| let result = WorkerConfigBuilder::default() | ||
| .namespace("default") | ||
| .task_queue("test-queue") | ||
| .versioning_strategy(default_versioning_strategy()) | ||
| .task_types(WorkerTaskTypes::activity_only()) | ||
| .max_cached_workflows(10usize) | ||
| .build(); | ||
|
|
||
| assert!( | ||
| result.is_err(), | ||
| "Workflow cache > 0 without workflows should fail" | ||
| ); | ||
| let err = result.err().unwrap().to_string(); | ||
| assert!( | ||
| err.contains("max_cached_workflows"), | ||
| "Error should mention max_cached_workflows: {err}", | ||
| ); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_all_combinations() { | ||
| let combinations = [ | ||
| (WorkerTaskTypes::workflow_only(), "workflows only"), | ||
| (WorkerTaskTypes::activity_only(), "activities only"), | ||
| (WorkerTaskTypes::nexus_only(), "nexus only"), | ||
| ( | ||
| WorkerTaskTypes { | ||
| enable_workflows: true, | ||
| enable_activities: true, | ||
| enable_nexus: false, | ||
| }, | ||
| "workflows + activities", | ||
| ), | ||
| ( | ||
| WorkerTaskTypes { | ||
| enable_workflows: true, | ||
| enable_activities: false, | ||
| enable_nexus: true, | ||
| }, | ||
| "workflows + nexus", | ||
| ), | ||
| ( | ||
| WorkerTaskTypes { | ||
| enable_workflows: false, | ||
| enable_activities: true, | ||
| enable_nexus: true, | ||
| }, | ||
| "activities + nexus", | ||
| ), | ||
| (WorkerTaskTypes::all(), "all types"), | ||
| ]; | ||
|
|
||
| for (task_types, description) in combinations { | ||
| let config = WorkerConfigBuilder::default() | ||
| .namespace("default") | ||
| .task_queue("test-queue") | ||
| .versioning_strategy(default_versioning_strategy()) | ||
| .task_types(task_types) | ||
| .build() | ||
| .unwrap_or_else(|e| panic!("Failed to build config for {description}: {e:?}")); | ||
|
|
||
| let effective = config.task_types; | ||
| assert_eq!( | ||
| effective, task_types, | ||
| "Effective types should match for {description}", | ||
| ); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -738,6 +738,12 @@ typedef struct TemporalCoreTunerHolder { | |
| struct TemporalCoreSlotSupplier nexus_task_slot_supplier; | ||
| } TemporalCoreTunerHolder; | ||
|
|
||
| typedef struct TemporalCoreWorkerTaskTypes { | ||
| bool enable_workflows; | ||
| bool enable_activities; | ||
| bool enable_nexus; | ||
| } TemporalCoreWorkerTaskTypes; | ||
|
|
||
| typedef struct TemporalCorePollerBehaviorSimpleMaximum { | ||
| uintptr_t simple_maximum; | ||
| } TemporalCorePollerBehaviorSimpleMaximum; | ||
|
|
@@ -765,7 +771,7 @@ typedef struct TemporalCoreWorkerOptions { | |
| struct TemporalCoreByteArrayRef identity_override; | ||
| uint32_t max_cached_workflows; | ||
| struct TemporalCoreTunerHolder tuner; | ||
| bool no_remote_activities; | ||
| struct TemporalCoreWorkerTaskTypes task_types; | ||
|
Member
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. This doesn't need to be a whole separate struct IMO just for the few fields within, but it's mostly harmless |
||
| uint64_t sticky_queue_schedule_to_start_timeout_millis; | ||
| uint64_t max_heartbeat_throttle_interval_millis; | ||
| uint64_t default_heartbeat_throttle_interval_millis; | ||
|
|
||
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 figure just three bools is simplest, even if you have to make a hash internally, but I guess not a big deal. Also at this point it could be argued it shouldn't have a default and should be required to be set.
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 wanna say having default be all by default matches existing behavior today, but then again not sure it matters much since it's not user facing
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.
No strong opinion on whether we should have a default since every SDK needs to start setting this anyways and the default therefore will never be used if done properly by SDKs
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.
fair point, I can remove the default to force SDKs to be explicit
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.
FWIW, I'm ok w/ defaulted too, just that I hope SDKs will ignore that and always set.