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".

Best regards,
Piotr Sikora
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel