Skip to content

Conversation

@bquorning
Copy link
Contributor

@bquorning bquorning commented Mar 5, 2025

While reading https://byroot.github.io/ruby/performance/2025/03/04/the-pitchfork-story.html, I noticed this sentence:

Now that I could more significantly change the server, I decided to move the responsibility of spawning new workers out of the master process, which I renamed “monitor process” for the occasion.

I had always been a bit confused by the code and documents using both “master” and “monitor” terms, and honestly thought they meant two different things.

Now, in this PR, the first commit changes docs and comments to always use “monitor”.

The 2nd commit changes (almost) all mentions of “Unicorn” into “Pitchfork” – but of course not when mentioning Pitchfork’s history, etc.

There’s still some mentions of “master”, e.g. HttpServer#master_sleep, HttpServer#awaken_master, Worker#register_to_master, and a @master_pid ivar. I’m happy to change these too, if they are not deemed breaking changes. (The HttpServer methods are private, so they should be safe to change). Added into the first commit.

@bquorning bquorning force-pushed the words branch 2 times, most recently from 206a3f2 to 89fc2ad Compare March 5, 2025 20:36
### Worker Processes

Note: the master uses a pipe to signal workers
Note: the monitor uses a pipe to signal workers
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically this is also outdated, as it is now a socketpair, but functionally that doesn't change much.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this PR I’d like to focus just on simple renaming, and keep other doc updates for another PR. 😅

@byroot
Copy link
Contributor

byroot commented Mar 5, 2025

LGTM. cc @etiennebarrie (if you can merge when you get a minute)

It seems from 5bcb768 that the master
process should be renamed to the monitor process.
@bquorning
Copy link
Contributor Author

I squashed all master → monitor changes together into the first commit and force-pushed. Sorry for any reviewer inconvenience that may have caused.

@etiennebarrie etiennebarrie merged commit 3a2f245 into Shopify:master Mar 6, 2025
10 checks passed
@bquorning bquorning deleted the words branch May 15, 2025 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants