[GitHub] [trafficserver] maskit commented on a diff in pull request #8818: Expose setting some HTTP/2 tunables via sni.yaml

[GitBox <gi...@apache.org>]

maskit commented on code in PR #8818:
URL: https://github.com/apache/trafficserver/pull/8818#discussion_r866459276


##########
iocore/net/I_NetVConnection.h:
##########
@@ -187,6 +187,8 @@ struct NetVCOptions {
   uint32_t packet_tos;
   uint32_t packet_notsent_lowat;
 
+  uint32_t http2_buffer_water_mark;

Review Comment:
   I'm ok with exposing a setting tunable via sni.yaml and I'm not going to block this PR because we can change the internal implementation later without breaking compatibility, but I don't like to have this option here because it's for a specific application protocol (not for generic connections).
   
   I'd put it like:
   
   ```
   class TLSSNISupport {
     ...
     struct HintsFromSNI { // NEW
       uint32_t http2_buffer_water_mark;
     } hints_from_sni;
   };
   ```
   
   It'd still have an application specific setting in one of TLS stuff, but it's better than contaminating NetVCOptions, IMO. And it also suggests the value is just a hint, and you can ignore it if appropriate (remember connection coalescing?).



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: github-unsubscribe@trafficserver.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org