Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29344646: Issue 4073 - Improve Nginx integration with "upstart" service provider (Closed)

Created:
May 26, 2016, 8:47 p.m. by mathias
Modified:
June 7, 2016, 10:04 a.m.
Reviewers:
Wladimir Palant
CC:
darkue, Felix Dahlke
Visibility:
Public.

Description

Issue 4073 - Improve Nginx integration with "upstart" service provider

Patch Set 1 #

Total comments: 10

Patch Set 2 : Issue 4073 - Address feedback from code-review #

Patch Set 3 : Issue 4073 - Address feedback from code-review #

Unified diffs Side-by-side diffs Delta from patch set Stats (+10 lines, -2 lines) Patch
M modules/nginx/manifests/init.pp View 1 2 1 chunk +10 lines, -2 lines 0 comments Download

Messages

Total messages: 9
mathias
May 26, 2016, 8:47 p.m. (2016-05-26 20:47:49 UTC) #1
Wladimir Palant
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp#newcode49 modules/nginx/manifests/init.pp:49: require => Package['nginx'], If I see it correctly, you ...
May 27, 2016, 11:15 a.m. (2016-05-27 11:15:08 UTC) #2
mathias
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp#newcode49 modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 11:15:08, Wladimir Palant wrote: ...
May 27, 2016, 11:43 a.m. (2016-05-27 11:43:54 UTC) #3
Wladimir Palant
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp#newcode49 modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 11:43:53, mathias wrote: > ...
May 27, 2016, 12:33 p.m. (2016-05-27 12:33:01 UTC) #4
mathias
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp#newcode49 modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 12:33:01, Wladimir Palant wrote: ...
May 27, 2016, 2:16 p.m. (2016-05-27 14:16:17 UTC) #5
Wladimir Palant
https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp File modules/nginx/manifests/init.pp (right): https://codereview.adblockplus.org/29344646/diff/29344647/modules/nginx/manifests/init.pp#newcode49 modules/nginx/manifests/init.pp:49: require => Package['nginx'], On 2016/05/27 14:16:16, mathias wrote: > ...
May 27, 2016, 2:41 p.m. (2016-05-27 14:41:34 UTC) #6
mathias
See also https://issues.adblockplus.org/ticket/4082 --
May 27, 2016, 3:32 p.m. (2016-05-27 15:32:06 UTC) #7
mathias
Avoiding seemingly unrelated change, though I still insist on this not being unlrelated to "improving ...
May 27, 2016, 4:03 p.m. (2016-05-27 16:03:42 UTC) #8
Wladimir Palant
May 30, 2016, 10:45 a.m. (2016-05-30 10:45:54 UTC) #9
On 2016/05/27 16:03:42, mathias wrote:
> The sleep command might require a
> longer delay under load, but always be a good guessed balance between log
> spamming and luck anyway, at best.

Frankly, I'm not sure whether the sleep command is necessary at all, I added it
merely as a precaution. If the signals are never reordered (which I think they
are) then the nginx process will always run the new process before shutting
down. The only issue might be that for a short period the new process won't be
ready to respond to requests, yet the old one won't be responding any more.
However, the server socket won't be shut down meaning that connection requests
will simply queue up. These will be processed either way as soon as the new
process is ready.

> Also the path to the pid-file being hard-coded is of course not optimal,

Another option would be running `pgrep -u root nginx` which will not be affected
by stale PID files as a nice side-effect (note that the master process is the
only one running under user root).

LGTM but you are free to implement additional changes to address the issues
above.

Powered by Google App Engine
This is Rietveld