[PATCH] Core: Configurable 'npoints' for ngx_http_upstream_hash

[Roman Arutyunyan]

Hello Koby,

> On 06 Dec 2015, at 08:57, Koby Nachmany <emptydocks at gmail.com> wrote:
> 
> 
> Hello!
> 
> I have a use case for an even, consistent balancing of a caching layer upstream cluster. I.e using the "Ketama algorithm"
> Current consistent hashing implementation in ngx_http_upstream_hash is hard-coded to '160' vbuckets  and real world results show a 20% variance in balancing, which is not acceptable in our case.

Please describe the test case to see 20% variance.
How many servers do you have in the upstream?

> Following is a patch (Thanks to agentz) that will allow a configurable vbuckets configuration param. Default will remain the same = 160. 
> Please consider pushing this "upstream" . No pun intended ;)
> 
> Koby N
> 
> --- a/src/http/modules/ngx_http_upstream_hash_module.c	2015-07-15 00:46:06.000000000 +0800
> +++ b/src/http/modules/ngx_http_upstream_hash_module.c	2015-10-11 22:26:47.952670175 +0800
> @@ -23,6 +23,7 @@ typedef struct {
>  
>  
>  typedef struct {
> +    ngx_uint_t                          npoints;
>      ngx_http_complex_value_t            key;
>      ngx_http_upstream_chash_points_t   *points;
>  } ngx_http_upstream_hash_srv_conf_t;
> @@ -66,7 +67,7 @@ static char *ngx_http_upstream_hash(ngx_
>  static ngx_command_t  ngx_http_upstream_hash_commands[] = {
>  
>      { ngx_string("hash"),
> -      NGX_HTTP_UPS_CONF|NGX_CONF_TAKE12,
> +      NGX_HTTP_UPS_CONF|NGX_CONF_TAKE123,
>        ngx_http_upstream_hash,
>        NGX_HTTP_SRV_CONF_OFFSET,
>        0,
> @@ -296,7 +297,10 @@ ngx_http_upstream_init_chash(ngx_conf_t
>      us->peer.init = ngx_http_upstream_init_chash_peer;
>  
>      peers = us->peer.data;
> -    npoints = peers->total_weight * 160;
> +
> +    hcf = ngx_http_conf_upstream_srv_conf(us, ngx_http_upstream_hash_module);
> +
> +    npoints = peers->total_weight * hcf->npoints;
>  
>      size = sizeof(ngx_http_upstream_chash_points_t)
>             + sizeof(ngx_http_upstream_chash_point_t) * (npoints - 1);
> @@ -355,7 +359,7 @@ ngx_http_upstream_init_chash(ngx_conf_t
>          ngx_crc32_update(&base_hash, port, port_len);
>  
>          prev_hash.value = 0;
> -        npoints = peer->weight * 160;
> +        npoints = peer->weight * hcf->npoints;
>  
>          for (j = 0; j < npoints; j++) {
>              hash = base_hash;
> @@ -391,7 +395,6 @@ ngx_http_upstream_init_chash(ngx_conf_t
>  
>      points->number = i + 1;
>  
> -    hcf = ngx_http_conf_upstream_srv_conf(us, ngx_http_upstream_hash_module);
>      hcf->points = points;
>  
>      return NGX_OK;
> @@ -657,6 +660,19 @@ ngx_http_upstream_hash(ngx_conf_t *cf, n
>      } else if (ngx_strcmp(value[2].data, "consistent") == 0) {
>          uscf->peer.init_upstream = ngx_http_upstream_init_chash;
>  
> +        if (cf->args->nelts > 3) {
> +            hcf->npoints = ngx_atoi(value[3].data, value[3].len);
> +
> +            if (hcf->npoints == (ngx_uint_t) NGX_ERROR) {
> +                ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> +                                   "invalid npoints parameter \"%V\"", &value[3]);
> +                return NGX_CONF_ERROR;
> +            }
> +
> +        } else {
> +            hcf->npoints = 160;
> +        }
> +
>      } else {
>          ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
>                             "invalid parameter \"%V\"", &value[2]);

The patch looks more or less ok.  However, I’d probably make a named parameter points=XXX instead.

--
Roman Arutyunyan