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

[Maxim Dounin]

Hello!

On Wed, Sep 11, 2013 at 03:46:51PM +0300, Greg Vishnepolsky wrote:

> Hi Maxim, thanks for the prompt reply!
> 
> > 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.
> 
> You're definitely right about this, I haven't thought about that configure
> option. How do you suggest to decouple the code? Perhaps add some kind of
> callback to the proxy configuration and expose a setter interface?

I think right aproach would be to expose some kind of "default" 
upstream which can be used by modules / configured by users.  Not 
sure how exactly this should be done from user point of view 
though.

> > 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.
> 
> When you're saying "is already present", are you referring to the code in
> "ngx_http_upstream_get_keepalive_peer", where "item->sockaddr" is being
> compared, as the key to the connection cache?
> If so, I'll try to see if it works in the described case. Perhaps a
> hostname should be added as another "uniqueness" identifier to this cache
> in addition to "sockaddr"? Then a single
> "ngx_http_upstream_keepalive_srv_conf_t" can be used for many hosts?
> If you believe that this should work, I agree that this is a better way to
> do the patch.

Yes.  The sockaddr contains information needed to identify a peer, 
and it's already used in multi-server upstream blocks for this.  

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