Hey Maxim, On Tue, Jul 25, 2017 at 6:28 PM, Piotr Sikora <piotrsik...@google.com> wrote: > Hey Maxim, > >> There are serious concerns about this and with other patches. >> Most generic ones are outlined below. >> >> 1. Server-side HTTP/2 code complexity concerns. >> >> As per discussion with Valentin, introducing client-related code >> paths in the server HTTP/2 code seems to complicate things. Given >> that the complexity of the code is already high, it might be >> better idea to implement separate client-side processing instead. > > I strongly disagree. Changes introduced to ngx_http_v2.c and > ngx_http_v2_filter_module.c are minimal. > > Also, having separate client-side and server-side code would result in > thousands of lines of duplicated code, for no reason, really. > >> 2. Different protocols in proxy module. >> >> You've introduced a completely different protocol in the proxy >> module, which contradicts the established practice of using >> separate modules for different protocols. >> >> Reasons for having all (or at least may of) the protocols >> available in the proxy module instead are well understood, and >> there is a long-term plan to revise current practice. The plan is >> to preserve protocol-specific modules as a separate entities, but >> let them share the common configuration directives, similar to how >> it is currently done with upstream{} blocks and the overall >> upstream infrastructure. So one can write something like >> >> proxy_pass fastcgi://127.0.0.1:9000; >> >> and get an expected result. >> >> While benefits of having all protocols sharing the same >> user-visible interface are understood, approach taken with HTTP/2 >> implementation is considered suboptimal, and will likely lead to >> something hard to maintain. >> >> I see two possible solutions here: >> >> - make HTTP/2-to-upstreams a separate full-featured upstream-based >> module, similar to proxy or fastcgi; > > But that's basically: > > cat ngx_http_proxy_module.c \ > | sed 's/ngx_http_proxy/ngx_http_proxy_v2/g' \ > > ngx_http_proxy_v2_module.c > > I don't see how copying ~4500 lines of code is a good idea. > > Also, as mentioned elsewhere, one of the reasons for adding HTTP/2 > code to the proxy module was the ability to negotiate HTTP/1.1 or > HTTP/2 via ALPN. Creating a separate HTTP/2-to-upstreams module won't > allow to add such feature in the future. > >> - start working on the different protocols in the proxy module, >> and introduce HTTP/2 proxying after this work is done. > > How is that even an option here? It's going to take forever before > such rewrite is done, and I have no desire nor time to work on that. > >> Additionally, there are also some minor / related comments: >> >> - Parts of the code, tightly coupled with upstream infrastructure, >> notably ngx_http_v2_upstream_output_filter(), are placed into >> v2/ directory. Instead, they seem to belong to the >> HTTP/2-to-upstream module implementation, as suggested in (1). > > Sure, this and a few other functions could be added to different files. > >> - Upstream infrastructure as available in >> src/http/ngx_http_upstream.c is expected to be >> protocol-agnostic. Introducing calls like >> ngx_http_v2_init_connection() there is a layering violation. >> Instead, there should be something more generic. > > I agree that ngx_http_v2_init_connection() isn't perfect, however, > fake connections are an abstraction layer that needs to be added > somewhere. The same is done for SSL, and it's somehow acceptable. > > I'm happy to use whatever generic mechanism is available, but there is > none at the moment, and I don't see how adding even more code and > complexity to this already big patchset is going to get us anywhere. > >> - Doing protocol parsing elsewhere instead of parsing things based >> on what nginx got from the connection results in some generic >> upstream mechanisms rendered not working - notably, it is no >> longer possible to simply write headers to a cache file. >> Serialization introduced instead, at least in its current form, >> looks more like a bandaid. > > Except that HTTP/2 headers as read from the wire, even if parsed in > separate module, couldn't be simply written to a cache file, because > of stateful HPACK encoding, so serialization would need to be done > anyway. > > Anyway, it appears that your "serious concerns" are mostly about > organization of the code, and not the code itself. What's unclear to > me is how much of the code review this patchset actually received, > i.e. if the existing code would be moved to a separate > HTTP/2-to-upstreams module, would it be acceptable or do you have > other issues with the code?
Ping. Best regards, Piotr Sikora _______________________________________________ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel