Fwd: Automatic pooling of upstream keepalive connections (patch proposal)

[Maxim Dounin]

Hello!

On Wed, Sep 11, 2013 at 10:27:20AM +0300, Greg Vishnepolsky wrote:

[...]

> These are to be configurations of the ngx_http_proxy_module. In the
> proposed patch, upstreams are created dynamically for new hosts (up to
> "max_hosts" at the same time). For each upstream the number of simultaneous
> keepalive'd connections would be "max_connections".
> The following patch does not currently solve the SSL session reuse problem,
> but it does handle the keepalive pooling problem. Here it is on github:
> https://gist.github.com/gregvish/6511822/raw/1f7de28c8ac7bf133376eccee9bb4fc65a8a2917/default_keepalive_patch
> It was taken against the 1.4.2 version source code.
> 
> Most of the new code was added to the "ngx_http_upstream_keepalive_module".
> This new code is called by the function
> "ngx_http_upstream_resolve_handler", after the call to
> "ngx_http_upstream_create_round_robin_peer". The new function I added,
> "ngx_http_upstream_default_keepalive_adapt_peer" converts the peer that was
> created by "create_round_robin_peer" into a "keepalive'd" peer (if the
> "conversion" fails, the peer remains a regular round robin one, and
> continues to work).

While the patch may work, it looks bad from architectural point of 
view.  It essentially makes upstream keepalive module an integral 
part of the upstream module, which isn't a good thing (and also 
will break --without-http_upstream_keepalive_module).  The 
upstream module should provide an interface to do things instead.

Also, it looks like the patch adds lots of code duplication.  
The code to check peer address and lookup a connection in the 
cache is already present in the upstream keepalive module, and it 
should be used instead of adding another structures/code to do the 
same task.

-- 
Maxim Dounin
http://nginx.org/en/donation.html