Throttle enumerator#45
Conversation
| end | ||
|
|
||
| def should_throttle? | ||
| @throttle_on.call |
There was a problem hiding this comment.
Is it worth being defensive around this being a proc? What about returning a boolean?
There was a problem hiding this comment.
Do you mean validating the return of a proc? Or validating that @throttle_on is a proc?
There was a problem hiding this comment.
@moechaieb are you saying not passing a proc as the throttle_on variable?
There was a problem hiding this comment.
Do you mean validating the return of a proc? Or validating that @throttle_on is a proc?
I'm asking if it's worth doing both. I'm usually not in favour of overly defensive programming. I think you know best what to do here, and my comment was just a suggestion.
There was a problem hiding this comment.
It's not always a proc, it can be anything that responds to call like:
"".method(:upcase)
=> #<Method: String#upcase>We could make it a bit more friendly and raise if the thing doesn't respond to call, but I find it pretty unlikely scenario to optimize for.
There was a problem hiding this comment.
The problem with passing something that is precomputed is that it would be the same value for all the iteration cycle of that job.
There was a problem hiding this comment.
For a string in my example, yes but for Database::Status.method(:healthy?) it kinda doesn't matter because the method has no state.
More over, throttle_on: Database::Status.method(:healthy?) vs throttle_on: -> { Database::Status.healthy? } saves you an extra stack.
moechaieb
left a comment
moechaieb
left a comment
There was a problem hiding this comment.
Simple enough. Thanks for extracting this Kir
|
I can take care of deploying it and updating core with the latest version. |
3 participants
Let's finally extract throttle enumerator from shopify/shopify. It might be useful for other apps that are building something that's supposed to throttle on DB health. I'm specifically thinking of https://github.com/Shopify/shopify-cloud/pull/155 here.