-
Notifications
You must be signed in to change notification settings - Fork 22
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
Spawn molds instead of promoting workers #42
Conversation
fac52cb
to
9db8728
Compare
f95b564
to
7ff0f0b
Compare
7ff0f0b
to
dba9ba3
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.
Cool, neat idea
docs/REFORKING.md
Outdated
``` | ||
|
||
When a reforking is triggered, one of the workers is selected to become a `mold`, and is taken out of rotation. | ||
Once the `mold` is done loading, the `master` ask it to spawn the desired number of workers: |
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.
Once the `mold` is done loading, the `master` ask it to spawn the desired number of workers: | |
Once the `mold` is done loading, the `master` asks it to spawn the desired number of workers: |
``` | ||
|
||
When a reforking is triggered, one of the workers is selected to become a `mold`, and is taken out of rotation. | ||
Once the `mold` is done loading, the `master` ask it to spawn the desired number of workers: | ||
|
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.
Looks like you're missing the ``` backticks to start the codeblock
refute_nil second_mold | ||
assert_equal 42, second_mold.pid | ||
|
||
assert_equal [first_mold, second_mold], @children.molds |
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's possible I've misunderstood the intent, but this test doesn't explicitly reap any molds, it seems to assert that when a new mold is spawned, the @mold
replaces the old with the new.
Should we change the name of the test or perhaps add something like:
diff --git a/test/unit/test_children.rb b/test/unit/test_children.rb
index b0313bc2..34808f67 100644
--- a/test/unit/test_children.rb
+++ b/test/unit/test_children.rb
@@ -78,6 +78,11 @@ def test_reap_old_molds
assert_equal 42, second_mold.pid
assert_equal [first_mold, second_mold], @children.molds
+
+ @children.reap(24)
+
+ assert_equal [second_mold], @children.molds
+ assert_equal second_mold, @children.mold
end
def test_reap_pending_mold
dba9ba3
to
9a2fe1a
Compare
Thanks @paarthmadan, all good points. |
The big benefit is we no longer reduce capacity when a promotion is happening. The downside is that complexity is a bit increased.
9a2fe1a
to
1df3007
Compare
The big benefit is we no longer reduce capacity when a promotion is happening.
The downside is that complexity is a bit increased.
Another important change is that since we fork, the various
Process._fork
callbacks will be invoked. For some it will be a positive (e.g. dropped connections), for others it may be somewhat negative (e.g. restarted background threads).This was suggested by @matthewd.