Add global enable_listen_notify configuration to disable both notify and listen #810
Conversation
There was a problem hiding this comment.
@mitchellhenke I think all the pieces are here, but I left some comments about the particulars.
Also, this led me to create #811, which is very out of scope for your change, but I wanted to document the other observations I had too, in case it provides some more context for your changes.
lib/good_job/configuration.rb
Outdated
| ActiveModel::Type::Boolean.new.cast( | ||
| options[:enable_listen_notify] || | ||
| rails_config[:enable_listen_notify] || | ||
| env['GOOD_JOB_ENABLE_LISTEN_NOTIFY'] || | ||
| DEFAULT_ENABLE_LISTEN_NOTIFY | ||
| ) |
There was a problem hiding this comment.
Because these are Booleans, I think this logic needs to do cast-to-boolean and then return the value if it isn't nil, for each and every value.
Right now I think it's possible for false values to not be set if passed in through rails_config.
There was a problem hiding this comment.
ahhhhh, yep, thank you
lib/good_job/notifier.rb
Outdated
| @@ -165,6 +166,8 @@ def create_executor | |||
| end | |||
|
|
|||
| def listen(delay: 0) | |||
| return unless @enable_listening | |||
There was a problem hiding this comment.
This can't be an early return. Creating the future is necessary because the Notifier is a bit more of a general purpose Reactor now with more responsibilities than just LISTENing e.g. this background thread creates/manages the Process record.
It will be messier but I think the unless @enable_listening needs to go within the Future/thread:
- Calling
connection.execute("LISTEN #{CHANNEL}")and the ActiveSupport Notification wrapping it, though the callback trigger needs to remain - Within
#wait_for_notifyhaving!@enable_listeningthat cause the logic to fall down to thesleep WAIT_INTERVALpath - Calling
connection.execute("UNLISTEN *")
There was a problem hiding this comment.
Thanks for explaining, I've added a couple commits to do the above and also tried avoiding checking out the connection. I'm not sure if it's right, and I couldn't figure out how to test it. notifier.connection was always nil whether or not enable_listening was enabled.
|
@mitchellhenke I just pushed up a few tweaks to this. I'm happy with where it ended up, but I'd love your eyes on it one more time before I merge it. Also, I just wanted to document that this will mean that the Probe Server's |
Thanks! It makes sense to me.
Ohhh, I wouldn't want to break that. I'm not using it currently, but might in the future. I think I understand this a bit better now. My primary goal at the outset was to limit the herd of queue selection queries that happens when enqueuing a lot of jobs (not necessarily in bulk). It could be a lot simpler and still accomplish the goal by adding a |
I agree on the simpler aspect of getting this closer to the individual job, but we aren't able to (or I'm very reluctant to) modify the Adapter interface. I'm thinking instead that it could be done as a job extension so that you could do something like this: MyJob.set(good_job_notify: false).perform_laterDoes that more closely match your needs? I think I can whip that up pretty quick. |
|
@mitchellhenke I whipped up that extension 😄 : #814 |
Thank you! Is it possible to opt in to not notifying for all jobs? The limitation on retries has me worry about a cascade of failures becoming a cascade of notifications on retry. |
|
@mitchellhenke I updated #814 to preserve the setting across retries. |
|
@mitchellhenke I updated the PR to also be serialized/preserved into the job data so that it will persist across job retries. |
|
Whoops, didn't mean to close this. |
|
@bensheldon yeah, that looks good to me! |
…configuration # Conflicts: # lib/good_job/notifier.rb
…configuration # Conflicts: # lib/good_job/adapter.rb
bensheldon
changed the title
Based on discussion in #731 (comment), this is a draft implementation of allowing the LISTEN/NOTIFY functionality to be configurable
I need to add some tests, but I wasn't sure on my initial approach.