Hi,
Attached is a patch which allows both custom headers and custom response
codes to be stored in memcached and returned by the memcached module.
The Nginx 0.6 version of this patch has been in production on a heavily
loaded site (100M unique visitors per month) for over a year with no
problems whatsoever. The original thread on this can be found here:
http://www.ruby-forum.com/topic/160379#724717
I think it would be a valuable addition to Nginx to include this in the
main distribution, but I’ll leave this up to Igor. The patch may be a
bit too hacky for his taste.
Feedback appreciated!
On Fri, Aug 21, 2009 at 10:06:00AM +0200, Spil G. wrote:
I think it would be a valuable addition to Nginx to include this in the
main distribution, but I’ll leave this up to Igor. The patch may be a
bit too hacky for his taste.
Feedback appreciated!
Attachments:
http://www.ruby-forum.com/attachment/3956/nginx-0.7.61-memcache.final.patch
Thank you for the patch.
Just a note - do you use memcached_pass_header and memcached_hide_header
?
I believe that memcached content is more controlled in comparison with
proxy/fastcgi, therefore there is no need in this functionality.
Hello!
On Fri, Aug 21, 2009 at 10:06:00AM +0200, Spil G. wrote:
I think it would be a valuable addition to Nginx to include this in the
main distribution, but I’ll leave this up to Igor. The patch may be a
bit too hacky for his taste.
Feedback appreciated!
Attachments:
http://www.ruby-forum.com/attachment/3956/nginx-0.7.61-memcache.final.patch
There are at least 2 problems with this patch I see:
-
It relies on ‘HTTP/’ prefix on to find out if key in question
has headers. This is bad, as there is no way to return text
starting from this string. IMHO it should be changed to use flags
instead.
-
It changes u->conf during request execution, this implies race
between different requests.
Maxim D.
Maxim D. wrote:
There are at least 2 problems with this patch I see:
-
It relies on ‘HTTP/’ prefix on to find out if key in question
has headers. This is bad, as there is no way to return text
starting from this string. IMHO it should be changed to use flags
instead.
-
It changes u->conf during request execution, this implies race
between different requests.
Great feedback. I understand the ‘HTTP/’ prefix thing, the flags thing
is a good idea. I’ll look into this.
Could you elaborate on the u->conf thing, because I’m not not at all an
Nginx expert. The patch basically consists of things I snatched from
other parts of Nginx (mostly from the proxy/fastcgi modules).
Thanks!
On Fri, Aug 21, 2009 at 12:47:24PM +0400, Maxim D. wrote:
problems whatsoever. The original thread on this can be found here:
http://www.ruby-forum.com/attachment/3956/nginx-0.7.61-memcache.final.patch
There are at least 2 problems with this patch I see:
- It relies on ‘HTTP/’ prefix on to find out if key in question
has headers. This is bad, as there is no way to return text
starting from this string. IMHO it should be changed to use flags
instead.
I agree, it should use a flag and the flag should not conflict with two
perl Cache-Memcached module’s flags:
F_STORABLE 0x0001
F_COMPRESS 0x0002
Igor S. wrote:
On Fri, Aug 21, 2009 at 10:06:00AM +0200, Spil G. wrote:
Just a note - do you use memcached_pass_header and memcached_hide_header
?
I believe that memcached content is more controlled in comparison with
proxy/fastcgi, therefore there is no need in this functionality.
We have not used those yet, but it seemed right to copy that behaviour
from proxy/fastcgi. There might be situations where you’d want to have
control over the headers being passed. It can be removed quite easily
though ofcourse.
Hello!
On Fri, Aug 21, 2009 at 11:04:23AM +0200, Spil G. wrote:
Great feedback. I understand the ‘HTTP/’ prefix thing, the flags thing
is a good idea. I’ll look into this.
Just a quick note: Tomash Brechko’s memcached_gzip patches should
have flags parsing code. Code should be available via
git.openhack.ru in nginx-patched.git, but it’s awfully slow and
I’m too lazy to search for exact link.
Could you elaborate on the u->conf thing, because I’m not not at all an
Nginx expert. The patch basically consists of things I snatched from
other parts of Nginx (mostly from the proxy/fastcgi modules).
I mean here:
% + /* disallow interception of error pages */
% + u->conf->intercept_404 = 0;
% + u->conf->intercept_errors = 0;
% + r->error_page = 1;
%
% return NGX_OK;
% }
% @@ -385,6 +563,8 @@
%
% u->headers_in.status_n = 404;
% u->state->status = 404;
% + u->conf->intercept_404 = 1;
% + u->conf->intercept_errors = 1;
%
% return NGX_OK;
% }
(btw, please use -p switch for diff, it simplifies reading patches
a lot)
You modified u->conf->intercept_404 and u->conf->intercept_errors
for this particular request. Meanwhile u->conf is structure
shared between all requests to this location (it’s configuration,
and supposed to be identical for all requests).
Looking closer - it should work without chance for any errors as
of now, since this is done just before it’s used in
ngx_http_upstream.c and there is no way how nginx could switch to
other request between setting them and checking (and threads in
nginx are broken now anyway).
Nevertheless I think it should be fixed somehow (e.g. by
duplicating relevant flags in request-specific data).
Maxim D.
On Fri, Aug 21, 2009 at 02:21:31PM +0400, Maxim D. wrote:
There are a lot more flags for memcached out there as used by
various clients (see hjp: zettel: memcached flags),
so it probably should be configurable instead.
Thank you for reference. I think we can use 7th bit (0x80) as default.
Hello!
On Fri, Aug 21, 2009 at 01:40:37PM +0400, Igor S. wrote:
The Nginx 0.6 version of this patch has been in production on a heavily
I agree, it should use a flag and the flag should not conflict with two
perl Cache-Memcached module’s flags:
F_STORABLE 0x0001
F_COMPRESS 0x0002
There are a lot more flags for memcached out there as used by
various clients (see hjp: zettel: memcached flags),
so it probably should be configurable instead.
Maxim D.
Thanks for all the tips. I’ll have a look at improving the patch.