-
Notifications
You must be signed in to change notification settings - Fork 656
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
Change director server from Thin to Puma #1800
Conversation
Hey Kiemes! Thanks for submitting this pull request! I'm here to inform the recipients of the pull request that you and the commit authors have already signed the CLA. |
[#150958544](https://www.pivotaltracker.com/story/show/150958544) Signed-off-by: Kai Hofstetter <[email protected]>
any numbers to compare?
…On Mon, Oct 2, 2017 at 8:00 AM, cfdreddbot ***@***.***> wrote:
Hey Kiemes!
Thanks for submitting this pull request! I'm here to inform the recipients
of the pull request that you and the commit authors have already signed the
CLA.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1800 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AALV9wKz2zaB2f-1sQbJE4NtaKiZRWbhks5soPqUgaJpZM4Pqxbx>
.
|
@voelzmo wanted this as starting point for further discussions. We have a follow-up story to get more numbers. At the moment we can just say that |
As an aside, Puma is the default web server for new Rails 5 apps since mid 2016 https://richonrails.com/articles/the-rails-5-0-default-files The history of Puma is that it was a fork of the unmaintained Mongrel - a threaded webserver - by the creator of Rubinius @evanphx. Puma has gone from strength to strength. |
We ran jmeter load tests against both a
Puma has significantly less |
thin_server.stop! | ||
end | ||
puma_configuration = Puma::Configuration.new do |user_config| | ||
user_config.workers 3 |
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.
Should this be configurable?
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.
at some point in the future, probably. Right now, we don't have a clear guidance on when to increase the number of workers and what that would mean for other things, such as your database connection pool. Therefore, I'd like to not expose this number for now.
The performance results are a nice addition to the PR! |
@evanphx we don't seems to do many fancy things with puma, how do you feel about our configuration bit:
Thanks! |
Don't merge yet, we've to fix some controller responses first which fail with puma. |
[#150958544](https://www.pivotaltracker.com/n/projects/1456570/stories/150958544) Signed-off-by: David Ansari <[email protected]> Signed-off-by: Felix Riegger <[email protected]>
DB connections are disconnected before puma forks its own worker processes. This ensures that all puma workers will get new DB connections. [#150958544](https://www.pivotaltracker.com/story/show/150958544) Signed-off-by: Beyhan Veli <[email protected]>
Signed-off-by: Felix Riegger <[email protected]>
We have fixed the controller responses and DB concurrency issues. The pr is now ready to be merged. |
@voelzmo Some quick notes on your config:
Basically, fix that url/port issue and you're good! |
We executed following performance tests:
Results are attached. |
Pumas bind and port config settings are mutually exclusive. Previously the bind setting was ignored and puma listened on all interfaces. [#151788691](https://www.pivotaltracker.com/story/show/151788691) Signed-off-by: Beyhan Veli <[email protected]>
Signed-off-by: Beyhan Veli <[email protected]>
@evanphx thank you for the feedback! We changed the binding as suggested. This should be ready now to be merged. |
#150958544
Signed-off-by: Kai Hofstetter [email protected]