-
-
Notifications
You must be signed in to change notification settings - Fork 210
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
Replace worker wording #409
Conversation
@bensheldon I tried looking into Concurrent source to customize thread name at its creation, but it was not isolated enough and would have required really dirty monkey patching. I greped worker and remaining mentions are relative to Puma or Heroku/Procfile. Only one mention of good_job/spec/support/reset_good_job.rb Line 63 in 1a7e4e9
It makes sense that thread name might not yet be renamed as this callback happens before we enter ScheduledTask block. Do you think we could rename the thread early enough to avoid this issue? |
59e4d85
to
c0bdc28
Compare
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.
Thanks for doing this! I have one requested change to undo the renaming within the Concurrent Ruby Executor.
lib/good_job/scheduler.rb
Outdated
# @private | ||
class ThreadPoolExecutor < Concurrent::ThreadPoolExecutor | ||
# Number of inactive threads available to execute tasks. | ||
# https://github.com/ruby-concurrency/concurrent-ruby/issues/684#issuecomment-427594437 | ||
# @return [Integer] | ||
def ready_worker_count | ||
def ready_thread_count |
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 name/code in this method are in the context of a Concurrent Ruby object, where I'd like to retain its internal usage of "worker".
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.
It makes sense, fixed
@@ -230,6 +230,7 @@ def create_executor | |||
# @return [void] | |||
def create_task(delay = 0) | |||
future = Concurrent::ScheduledTask.new(delay, args: [performer], executor: executor, timer_set: timer_set) do |thr_performer| | |||
Thread.current.name = Thread.current.name.sub("-worker-", "-thread-") if Thread.current.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.
Yes!!
c0bdc28
to
e8bdf9a
Compare
b672497
to
eba498e
Compare
I think it's fine to ignore. That logging is used solely for debugging. I agree that monkeypatching is excessive and would also impact the application globally outside of GoodJob, which we should avoid. |
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.
👏
Closes #406.
worker
bythread
in thread's name given by Concurrent::ThreadPoolExecutor, as soon as we get it inScheduledTask