-
Notifications
You must be signed in to change notification settings - Fork 15
Expose poller automation #275
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
Conversation
temporalio/ext/src/worker.rs
Outdated
initial: poller_behavior.member::<usize>(id!("initial"))?, | ||
} | ||
} else { | ||
PollerBehavior::SimpleMaximum(poller_behavior.member::<usize>(id!("maximum"))?) |
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.
PollerBehavior::SimpleMaximum(poller_behavior.member::<usize>(id!("maximum"))?) | |
PollerBehavior::SimpleMaximum(poller_behavior.member::<usize>(id!("simple_maximum"))?) |
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.
🤦♂️ thanks
temporalio/lib/temporalio/worker.rb
Outdated
workflow_task_poller_behavior: nil, | ||
activity_task_poller_behavior: nil, |
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.
Add these to docs
temporalio/lib/temporalio/worker.rb
Outdated
:debug_mode, | ||
:workflow_task_poller_behavior, | ||
:activity_task_poller_behavior |
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.
:debug_mode, | |
:workflow_task_poller_behavior, | |
:activity_task_poller_behavior | |
:workflow_task_poller_behavior, | |
:activity_task_poller_behavior, | |
:debug_mode |
Might as well keep the same order as initialize
(and update where you instantiate the options too)
temporalio/lib/temporalio/worker.rb
Outdated
# Convert deprecated max concurrent polls to poller behaviors if not specified | ||
workflow_task_poller_behavior ||= SimpleMaximumPollerBehavior.new(maximum: max_concurrent_workflow_task_polls) | ||
activity_task_poller_behavior ||= SimpleMaximumPollerBehavior.new(maximum: max_concurrent_activity_task_polls) |
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 thinking we should set these as parameter defaults. Ruby is cool in that not only does the default expression not get evaluated if a value is provided, but a default parameter expression can use parameters defined before it.
# Creates a simple maximum poller behavior | ||
# The poller will attempt to poll as long as a slot is available, up to the | ||
# provided maximum. Cannot be less than two for workflow tasks, or one for other tasks. | ||
# | ||
# @param maximum [Integer] Maximum number of concurrent poll requests. | ||
# @return [SimpleMaximumPollerBehavior] A simple maximum poller behavior | ||
def self.simple_maximum(maximum = 5) | ||
SimpleMaximumPollerBehavior.new(maximum: maximum) | ||
end | ||
|
||
# Creates an autoscaling poller behavior | ||
# The poller will automatically scale the number of pollers based on feedback | ||
# from the server. A slot must be available before beginning polling. | ||
# | ||
# @param minimum [Integer] Minimum number of poll calls (assuming slots are available). | ||
# @param maximum [Integer] Maximum number of poll calls that will ever be open at once. | ||
# @param initial [Integer] Number of polls attempted initially before scaling kicks in. | ||
# @return [AutoscalingPollerBehavior] An autoscaling poller behavior | ||
def self.autoscaling(minimum: 1, maximum: 100, initial: 5) | ||
AutoscalingPollerBehavior.new(minimum: minimum, maximum: maximum, initial: initial) | ||
end |
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.
# Creates a simple maximum poller behavior | |
# The poller will attempt to poll as long as a slot is available, up to the | |
# provided maximum. Cannot be less than two for workflow tasks, or one for other tasks. | |
# | |
# @param maximum [Integer] Maximum number of concurrent poll requests. | |
# @return [SimpleMaximumPollerBehavior] A simple maximum poller behavior | |
def self.simple_maximum(maximum = 5) | |
SimpleMaximumPollerBehavior.new(maximum: maximum) | |
end | |
# Creates an autoscaling poller behavior | |
# The poller will automatically scale the number of pollers based on feedback | |
# from the server. A slot must be available before beginning polling. | |
# | |
# @param minimum [Integer] Minimum number of poll calls (assuming slots are available). | |
# @param maximum [Integer] Maximum number of poll calls that will ever be open at once. | |
# @param initial [Integer] Number of polls attempted initially before scaling kicks in. | |
# @return [AutoscalingPollerBehavior] An autoscaling poller behavior | |
def self.autoscaling(minimum: 1, maximum: 100, initial: 5) | |
AutoscalingPollerBehavior.new(minimum: minimum, maximum: maximum, initial: initial) | |
end |
Probably don't need either of these if we're not hiding the subclasses. Ruby users have no problem instantiating classes in these cases and therefore the helpers do not provide much value
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.
Hmm, I added because I did this for VersioningOverride (same with the prefixed name) and we were fine with it there. I have no problem removing it though.
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 can fix those in this PR since it's unreleased if we want
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 see now that versioning override not only made the subclasses adjacent to the parent (usually Ruby prefers them inside) but that we added helpers that don't have too much value. Sorry I didn't notice at the time.
For Ruby users Temporalio::Worker::PollerBehavior.autoscaling(maximum: 500)
and Temporalio::Worker::PollerBehavior::Autoscaling.new(maximum: 500)
are effectively equivalent and I think most even prefer the latter. Would suggest moving the versioning behavior ones too (move subclass to inside parent, remove class name suffix, remove helper).
|
||
# A poller behavior that attempts to poll as long as a slot is available, up to the | ||
# provided maximum. Cannot be less than two for workflow tasks, or one for other tasks. | ||
class SimpleMaximumPollerBehavior < PollerBehavior |
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.
class SimpleMaximumPollerBehavior < PollerBehavior | |
class SimpleMaximum < PollerBehavior |
It is idiomatic in Ruby to remove this suffix (same for autoscaling one below)
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.
If so it seems like these should be inside a PollerBehavior
module or the class? B/c
Temporalio::Worker::Autoscaling.new
is super confusing
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 moved them inside the class.
b2da34b
to
f2feea4
Compare
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.
Nothing blocking
attr_reader :maximum | ||
|
||
# @param maximum [Integer] Maximum number of concurrent poll requests. | ||
def initialize(maximum: 5) |
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.
def initialize(maximum: 5) | |
def initialize(maximum = 5) |
Feel free to make this a positional arg if it makes more sense, which it seems to here.
attr_reader :maximum | ||
|
||
# @param maximum [Integer] Maximum number of concurrent poll requests. | ||
def initialize(maximum: 5) |
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.
Arguably this shouldn't have a default, but rather where it's used as a default can provide this parameter value. Maybe same for autoscaling. But not a big deal either way.
temporalio/test/worker_test.rb
Outdated
# Give pollers a beat to get started | ||
sleep(0.3) |
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 am worried about flakiness of this kind of assumption. Maybe something like assert_eventually
that asserts temporal_num_pollers
exists and returns that line?
5b925f7
to
60d1238
Compare
60d1238
to
9003ad2
Compare
What was changed
Expose poller automation options
Why?
SDK Parity
Checklist
Needs #274 to merge first
Closes
How was this tested:
Added test
Any docs updates needed?