-
-
Notifications
You must be signed in to change notification settings - Fork 286
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
Add argument support to #fire_event #426
Conversation
855661b
to
33dc7b9
Compare
lib/shoryuken/util.rb
Outdated
@@ -4,13 +4,13 @@ def logger | |||
Shoryuken.logger | |||
end | |||
|
|||
def fire_event(event, reverse = false) | |||
def fire_event(event, options = {}, *event_args) |
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.
Hi @hasbrettn
I prefer def fire_event(event, reverse = false, *event_args)
or def fire_event(event, reverse = false, *event_options)
to avoid introducing breaking changes.
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.
The latter event_options
is only for making it more explicit, like fire_event(:dispatch, false, queue_name: queue.name)
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.
Are you thinking:
def fire_event(event, reverse = false, event_options = {})
Such that event options is meant to be hash?
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.
Such that event options is meant to be hash?
@hasbrettn I was thinking on that for making the option more explicit, otherwise it would support fire_event(..., false, queue_name, something_else)
and fire_event(..., false, something_else, queue_name)
. If it becomes a hash, queue_name
will be always queue_name:
, no matter the order it was passed in. WDYT?
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 think that makes a lot of sense and I'm in support of that. I'll push a change.
@hasbrettn thanks for the contribution 🍻 Could you also give a use for passing the queue name? |
Thanks for the feedback! Our use case is that we'd like to push a stat to statsd each time the queue is polled to more easily distinguish between low-volume times, and down time. Because we're getting this up in jruby -> docker -> kubernetes, a lot of the normal process process identification things that you'd normally do to make sure your process is up is more difficult and involves multiple teams. This change makes it easy to identify that specific queues are actively being polled, rather than resorting to cloudwatch metrics. |
33dc7b9
to
8ce451f
Compare
I think I made the changes we talked about. Let me know if there's anything else that you want to see in here. 🥂 |
Awesome! Your change is out 3.1.9. Thanks 🍻 |
The main feature of this pull request is that it will add the queue name to the
:dispatch
lifecycle event.I can think of a couple different method signatures for
#fire_event
. I'm happy to consider other signatures if you have opinions about it.What I chose:
The other way that I considered: