Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(467)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 10 months ago by mathias
Modified:
3 years, 10 months ago
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
3 years, 10 months ago (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 ...
3 years, 10 months ago (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: ...
3 years, 10 months ago (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: > ...
3 years, 10 months ago (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: ...
3 years, 10 months ago (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: > ...
3 years, 10 months ago (2016-05-27 14:41:34 UTC) #6
mathias
See also https://issues.adblockplus.org/ticket/4082 --
3 years, 10 months ago (2016-05-27 15:32:06 UTC) #7
mathias
Avoiding seemingly unrelated change, though I still insist on this not being unlrelated to "improving ...
3 years, 10 months ago (2016-05-27 16:03:42 UTC) #8
Wladimir Palant
3 years, 10 months ago (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.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5