Skip to content
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

Support for server-side rate limiting of activities #46

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

robholland
Copy link
Contributor

@robholland robholland commented Feb 25, 2021

This adds support for server-side rate limiting of activities per
second for a given task queue.

Rob Holland added 2 commits February 25, 2021 17:57
This also adds support for server-side rate limiting of activities per
second for a given task queue.
@robholland robholland changed the title Allow varying options per activity task queue. Support for server-side rate limiting of activities Mar 2, 2021
@@ -9,7 +9,7 @@ module Temporal
class Worker
# activity_thread_pool_size: number of threads that the poller can use to run activities.
# can be set to 1 if you want no paralellism in your activities, at the cost of throughput.
def initialize(activity_thread_pool_size: Temporal::Activity::Poller::DEFAULT_OPTIONS[:thread_pool_size])
def initialize(activity_thread_pool_size: Temporal::Activity::Poller::DEFAULT_OPTIONS[:thread_pool_size], activity_max_tasks_per_second: nil)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a hash of options? There are quite a few options in the Go SDK for worker configuration that may be eventually added here. Changing this later would be a breaking change.

Both Java and Go SDKs name this "max task queue activities per second". Admittedly that does not match the field name in the proto. Because there is a distinction made between this limit and "max worker activities per second" too, it might make sense to add both options with explanatory comments while you're already in here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changing this to an options hash later would not break the API. That would be backward compatible with the current kwargs. Unless you mean doing initialize(options: { ... }) but I can't see what the value is in doing that? Happy to change the option name to be closer to the Go SDK one. I don't think we should add the other option as part of this PR though, it would require adding code to do the rate limiting that isn't related to the server side rate limiting.


worker.start

expect(activity_poller).to have_received(:start)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: I'm not sure this check is needed in this spec, we're only checking that the Poller is initialised with the new option

christopherb-stripe pushed a commit to christopherb-stripe/temporal-ruby that referenced this pull request Jan 19, 2022
…nal-with-start

Change how signal_with_start is exposed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants