Skip to content

Move unicorn_killer to Gemfile#6792

Merged
denschub merged 1 commit intodevelopfrom
unknown repository
May 6, 2016
Merged

Move unicorn_killer to Gemfile#6792
denschub merged 1 commit intodevelopfrom
unknown repository

Conversation

@ghost
Copy link
Copy Markdown

@ghost ghost commented Apr 13, 2016

I stumbled over that and though it is a good idea to use the gem file instead. There are a view changes since 2013.

There is also the possibility to kill workers by max_requests:

use Unicorn::WorkerKiller::MaxRequests, max_request_min, max_request_max

Do we need/want this? If so.. unfortunately I have no reference for some max/min values:
I found some examples with 500-600 request per worker.

related to #6269

Comment thread config.ru Outdated
# Kill unicorn workers really aggressively (at 300mb)
if defined?(Unicorn)
use UnicornKiller::Oom, 300 * 1024
require 'unicorn/worker_killer'
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

Comment thread config.ru
# Kill unicorn workers really aggressively (at 300mb)
if defined?(Unicorn)
use UnicornKiller::Oom, 300 * 1024
require "unicorn/worker_killer"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This require should be on top of the file.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not necessarily, since the defined?(Unicorn) makes unicorn optional with no changes besides the Gemfile.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I was more worried about codestyle here - requires in the middle of the code look weird. But if you're fine with it, I can get over it. ;)

Copy link
Copy Markdown
Member

@jhass jhass Apr 21, 2016

Choose a reason for hiding this comment

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

It's not too weird and I mean there's another require just above it still

@denschub denschub merged commit 56c7af9 into diaspora:develop May 6, 2016
denschub added a commit that referenced this pull request May 6, 2016
@denschub denschub added this to the 0.6.0.0 milestone May 6, 2016
@denschub
Copy link
Copy Markdown
Member

denschub commented May 6, 2016

Merged.

Do we need/want this?

If we do, we can always add that later. :)

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