Skip to content
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

Merged
merged 1 commit into from Oct 7, 2021

Conversation

Hugo-Hache
Copy link
Contributor

@Hugo-Hache Hugo-Hache commented Oct 7, 2021

Closes #406.

@Hugo-Hache
Copy link
Contributor Author

Hugo-Hache commented Oct 7, 2021

@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 -worker- related to GoodJob remains, from this log:

conn.exec_query("SET application_name = '#{thread_name}'", "Set application name")

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?

Copy link
Owner

@bensheldon bensheldon left a 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.

# @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
Copy link
Owner

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

Copy link
Contributor Author

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
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!!

@Hugo-Hache Hugo-Hache force-pushed the replace-worker-wording branch 2 times, most recently from b672497 to eba498e Compare October 7, 2021 14:31
@bensheldon
Copy link
Owner

Do you think we could rename the thread early enough to avoid this issue?

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.

Copy link
Owner

@bensheldon bensheldon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👏

@bensheldon bensheldon merged commit b2717bd into bensheldon:main Oct 7, 2021
@bensheldon bensheldon added the documentation Improvements or additions to documentation label Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Improvements or additions to documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rewrite Scheduler "worker" thread name to be thread
2 participants