changeset 45:489c5d4318ff draft

Keepalive: "single" parameter deprecated. The original idea was to optimize edge cases in case of interchangeable backends, i.e. don't establish a new connection if we have any one cached. This causes more harm than good though, as it screws up underlying balancer's idea about backends used and may result in various unexpected problems.
author Maxim Dounin <mdounin@mdounin.ru>
date Mon, 25 Jun 2012 22:55:53 +0400
parents d9ac9ad67f45
children 92125e266aa4
files README ngx_http_upstream_keepalive_module.c t/memcached-keepalive.t
diffstat 3 files changed, 9 insertions(+), 64 deletions(-) [+]
line wrap: on
line diff
--- a/README	Thu Sep 08 19:03:19 2011 +0400
+++ b/README	Mon Jun 25 22:55:53 2012 +0400
@@ -19,10 +19,6 @@
               room to cache new connections - last recently used connections
               will be kicked off the cache.
 
-            - single
-              Treat everything as single host.  With this flag connections
-              to different backends are treated as equal.
-
 Sample configuration:
 
     upstream memd {
--- a/ngx_http_upstream_keepalive_module.c	Thu Sep 08 19:03:19 2011 +0400
+++ b/ngx_http_upstream_keepalive_module.c	Mon Jun 25 22:55:53 2012 +0400
@@ -11,7 +11,6 @@
 
 typedef struct {
     ngx_uint_t                         max_cached;
-    ngx_uint_t                         single;       /* unsigned:1 */
 
     ngx_queue_t                        cache;
     ngx_queue_t                        free;
@@ -222,38 +221,11 @@
 
     kp->failed = 0;
 
-    /* single pool of cached connections */
-
-    if (kp->conf->single && !ngx_queue_empty(&kp->conf->cache)) {
-
-        q = ngx_queue_head(&kp->conf->cache);
-
-        item = ngx_queue_data(q, ngx_http_upstream_keepalive_cache_t, queue);
-        c = item->connection;
-
-        ngx_queue_remove(q);
-        ngx_queue_insert_head(&kp->conf->free, q);
-
-        ngx_log_debug1(NGX_LOG_DEBUG_HTTP, pc->log, 0,
-                       "get keepalive peer: using connection %p", c);
-
-        c->idle = 0;
-        c->log = pc->log;
-        c->read->log = pc->log;
-        c->write->log = pc->log;
-#if (NGX_UPSTREAM_KEEPALIVE_PATCHED)
-        c->pool->log = pc->log;
-#endif
-
-        pc->connection = c;
-        pc->cached = 1;
-
-        return NGX_DONE;
-    }
+    /* ask balancer */
 
     rc = kp->original_get_peer(pc, kp->data);
 
-    if (kp->conf->single || rc != NGX_OK) {
+    if (rc != NGX_OK) {
         return rc;
     }
 
@@ -599,7 +571,8 @@
     for (i = 2; i < cf->args->nelts; i++) {
 
         if (ngx_strcmp(value[i].data, "single") == 0) {
-            kcf->single = 1;
+            ngx_conf_log_error(NGX_LOG_WARN, cf, 0,
+                               "the \"single\" parameter is deprecated");
             continue;
         }
 
--- a/t/memcached-keepalive.t	Thu Sep 08 19:03:19 2011 +0400
+++ b/t/memcached-keepalive.t	Mon Jun 25 22:55:53 2012 +0400
@@ -20,7 +20,7 @@
 eval { require Cache::Memcached; };
 plan(skip_all => 'Cache::Memcached not installed') if $@;
 
-my $t = Test::Nginx->new()->has('rewrite')->has_daemon('memcached')->plan(17)
+my $t = Test::Nginx->new()->has('rewrite')->has_daemon('memcached')->plan(16)
 	->write_file_expand('nginx.conf', <<'EOF');
 
 %%TEST_GLOBALS%%
@@ -38,12 +38,6 @@
         keepalive 1;
     }
 
-    upstream memd2 {
-        server 127.0.0.1:8081;
-        server 127.0.0.1:8082;
-        keepalive 1 single;
-    }
-
     upstream memd3 {
         server 127.0.0.1:8081;
         server 127.0.0.1:8082;
@@ -71,11 +65,6 @@
             memcached_pass memd;
         }
 
-        location /memd2 {
-            set $memcached_key "/";
-            memcached_pass memd2;
-        }
-
         location /memd3 {
             set $memcached_key "/";
             memcached_pass memd3;
@@ -160,25 +149,12 @@
 is($memd1->stats()->{total}->{total_connections}, $total + 1,
 	'big head request closes connection');
 
-# two backends with 'single' option - should establish only one connection
+# two backends with maximum number of cached connections set to 1,
+# should establish new connection on each request
 
 $total = $memd1->stats()->{total}->{total_connections} +
 	$memd2->stats()->{total}->{total_connections};
 
-http_get('/memd2');
-http_get('/memd2');
-http_get('/memd2');
-
-is($memd1->stats()->{total}->{total_connections} +
-	$memd2->stats()->{total}->{total_connections}, $total + 1,
-	'only one connection with two backends and single');
-
-$total = $memd1->stats()->{total}->{total_connections} +
-	$memd2->stats()->{total}->{total_connections};
-
-# two backends without 'single' option and maximum number of cached
-# connections set to 1 - should establish new connection on each request
-
 http_get('/memd3');
 http_get('/memd3');
 http_get('/memd3');
@@ -187,8 +163,8 @@
 	$memd2->stats()->{total}->{total_connections}, $total + 3,
 	'3 connections should be established');
 
-# two backends without 'single' option and maximum number of cached
-# connections set to 10 - should establish only two connections (1 per backend)
+# two backends with maximum number of cached connections set to 10,
+# should establish only two connections (1 per backend)
 
 $total = $memd1->stats()->{total}->{total_connections} +
         $memd2->stats()->{total}->{total_connections};