Hello!

On Mon, Jun 19, 2017 at 07:31:11AM -0700, Piotr Sikora via nginx-devel wrote:

> Hey Maxim,
>
> > It doesn't look like this patch make sense by its own.
> >
> > If this patch is a part of a larger work, please consider
> > submitting a patch series with the whole feature instead.
> > Individual patches which doesn't make much sense
> > by its own are hard to review, and are likely to be rejected.
>
> Actually, it does, the only missing part (from HTTP/2 patchset) is:
>
> case NGX_HTTP_VERSION_11:
> ngx_str_set(&alpn, NGX_HTTP_11_ALPN_ADVERTISE);
> break;
> +
> +#if (NGX_HTTP_V2)
> + case NGX_HTTP_VERSION_20:
> + ngx_str_set(&alpn, NGX_HTTP_V2_ALPN_ADVERTISE);
> + break;
> +#endif
> }
>
> if (ngx_ssl_alpn_protos(cf, plcf->upstream.ssl, &alpn) != NGX_OK) {
>
> ...so this patch is perfectly reviewable on its own.
>
> In case you expected ALPN-negotiation - I didn't add it, since it
> would require rewrite of upstream logic, i.e. u->create_request()
> would need to be called after upstream is already connected, and
> possibly again in case of retries that negotiated different ALPN
> protocol, which would add complexity to the already big patchset.
> Also, I'm not sure how useful this is for upstream connections in
> reverse proxies.
>
> Let me know if that's a must-have, but IMHO we could always add
> "proxy_http_version alpn" in the future, without blocking this and
> HTTP/2 patchset, which effectively implement "prior knowledge".

I don't see how this patch is useful by its own, without
additional HTTP/2 patchset, and it clearly adds additional
user-visible complexity to the proxy module. And hence the
suggestion is to include this patch into the HTTP/2 patchset as it
is clearly a part of this patchset.

Note well that at this point it might be a good idea to implement
HTTP/2 to upstreams as a separate module, as it is done for other
protocols like FastCGI, SCGI, uWSGI, and so on, instead of trying
to built it into the proxy module which is intended to talk HTTP,
not HTTP/2. It will better follow the generic concept we use now,
"one protocol - one module", and will also imply much less
blocking in general.

--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel