Skip to content

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
# 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.

3 participants