search

zombocom / puma_worker_killer

Automatically restart Puma cluster workers based on max RAM available
748 stars 77 forks source link

Sleep before first reap #40

Closed daveallie closed 8 years ago

daveallie commented 8 years ago

When using AutoReap (mainly with the RollingRestart reaper), AutoReap will perform a reap when it is first loaded. This means that when using a the RollingRestart, as soon at the app is launched, a worker will get sent a SIGTERM (due to AutoReap triggering a reap).

This is pretty pointless, and can sometimes cause issues with workers starting twice for no reason when launching the app.

This change swaps the order the sleep and reap call, meaning that AutoReap will perform a sleep before its first reap, causing less double-starts of puma workers.

schneems commented 8 years ago

I think I did it in that order to make tests easier/quicker. But looks like tests work with it in that order, so let's :shipit:

schneems commented 8 years ago

Well...that broke master.

daveallie commented 8 years ago

Tests will most likely fail intermittently due to the randomness introduced to the frequency at https://github.com/schneems/puma_worker_killer/blob/master/lib/puma_worker_killer.rb#L26.

As the test has a timeout of 10 seconds, it can occasionally timeout without performing a rolling restart. I have two potential fixes:

Making the test timeout longer for this test: https://github.com/schneems/puma_worker_killer/blob/master/test/puma_worker_killer_test.rb#L46 like in this. (15 seconds should do the trick) or Making skip_first_reap a config option.

Let me know if you'd like the second option and I'll open a PR.

ghiculescu commented 8 years ago

having a configuration option that only exists for tests seems like a bad idea - increasing the timeout so it works with all tests (and more accurately reflects real world use) seems like a better idea.