Thank you Sudheer, this is helpful. Considering Masaori's input, it sounds like it would be wise to amend TSHttpTxnClientPriorityGet to just take a single, generic priority opaque structure:
tsapi TSReturnCode TSHttpTxnClientStreamIdGet(TSHttpTxn txnp, TSHttpProtocolType type, void *stream_id); tsapi TSReturnCode TSHttpTxnClientPriorityGet(TSHttpTxn txnp, TSHttpProtocolType type, void *priority); I'll work on a prototype of this and post it as a separate commit on the draft PR. On Tue, Sep 1, 2020 at 10:52 AM Sudheer Vinukonda <sudheervinuko...@yahoo.com.invalid> wrote: > >> (primary reason for that being let the user have "control" on what > they want to retrieve). > And of course, the API shall return TS_ERROR, if the passed in protocol > "type" is not correct for the given TXN. > On Tuesday, September 1, 2020, 08:45:21 AM PDT, Sudheer Vinukonda < > sudheervinuko...@yahoo.com.invalid> wrote: > > +1 to use opaque structures and use protocol "type" as the "key". > I'd slightly prefer the caller pass in the protocol type rather than > letting TS automatically determine that (primary reason for that being let > the user have "control" on what they want to retrieve). > For e.g > > tsapi TSReturnCode TSHttpTxnClientStreamIdGet(TSHttpTxn txnp, > TSHttpProtocolType type, void *stream_id); > tsapi TSReturnCode TSHttpTxnClientPriorityGet(TSHttpTxn > txnp, TSHttpProtocolType type, void *stream_dependency, void *weight); > > where, `TSHttpProtocolType` is a new interface we add with applicable HTTP > protocols defined and it is up to the caller to pass in appropriate data > types for any given protocol they call the API for. > > All that said, like Leif, I don't feel super strong on this as well > and, I'd be okay with the general consensus. > > Thanks, > Sudheer > > > > On Tuesday, September 1, 2020, 08:02:16 AM PDT, Leif Hedstrom < > zw...@apache.org> wrote: > > > > > On Sep 1, 2020, at 01:20, Masaori Koshiba <masa...@apache.org> wrote: > > > > I totally agree with less API is better. > > > >> tsapi TSReturnCode TSHttpTxnClientStreamIdGet(TSHttpTxn txnp, > > uint64_t *stream_id); > > > > Stripping "Http2" looks reasonable. We can use this API for protocols > that > > have stream id. > > > >> tsapi TSReturnCode TSHttpTxnClientPriorityGet(TSHttpTxn txnp, > > uint64_t *stream_dependency, int *weight); > > > > I doubt removing "Http2" from this API makes sense. Because this API is > > assuming HTTP/2's priority scheme (dependency & weight). > > As Masakazu pointed out, a new priority scheme is proposed and it's > > completely different from HTTP/2's priority scheme. > > It's trying to make a priority scheme HTTP version-independent. > > Rather than making the API protocol specific, why not use opaque > structures, and a protocol “type” identifier returned? Even with H2 and > later H3, it’s possible things like priority keeps changing. > > The caller would of course have to type cast to the appropriate priority > struct. However, I don’t feel super strongly here, and sadly we decided > not to turn the public APIs into C++ where this type of behavior is more > natural. > > — Leif > > > > So I'd like to leave "Http2" for this API and reserve > > "TSHttpTxnClientPriorityGet" for the new priority scheme. > > > > - Masaori > > > >> On Tue, Sep 1, 2020 at 12:02 PM Brian Neradt <brian.ner...@gmail.com> > wrote: > >> > >> Thanks all, this is good feedback. It sounds like we're in agreement on > >> dropping the explicit reference to Http2 so this can accommodate future > >> protocols. Considering this and the observation that stream_id is 62 > bits > >> for HTTP/3, the output parameters should be increased in size > accordingly. > >> That would make the API look like the following: > >> > >> tsapi TSReturnCode TSHttpTxnClientStreamIdGet(TSHttpTxn txnp, uint64_t > >> *stream_id); > >> > >> tsapi TSReturnCode TSHttpTxnClientPriorityGet(TSHttpTxn txnp, uint64_t > >> *stream_dependency, int *weight); > >> > >> This will satisfy the Traffic Dump plugin needs. The concern raised > about > >> protocol identification is not an issue for Traffic Dump because it will > >> know the protocol from either the protocol retrieval API > >> (TSHttpSsnClientProtocolStackGet) or from the version in the request > >> via TSHttpHdrVersionGet. That being the case, I'm content not adding a > >> separate output parameter for this. > >> > >> That said, I don't want to dismiss the option too quickly if others are > >> still interested. Sudheer, you seem to have the most developed ideas for > >> this. If, considering what I say above, you'd still like us to consider > the > >> inclusion of a context parameter, could you please flesh out: > >> > >> - What the signatures for the stream id and priority getters would look > >> like. > >> - The structure of the context object. > >> - Would all HTTP/2 and above APIs contain this context parameter? > >> > >> Otherwise, if we're OK not having the context parameter, I can update > the > >> draft PR with the above changes to the API. > >> > >> Thanks, > >> > >> On Mon, Aug 31, 2020 at 9:19 PM Sudheer Vinukonda > >> <sudheervinuko...@yahoo.com.invalid> wrote: > >> > >>> Right, it does make sense to have a generic API naming (and remove > http2 > >>> reference from it), but I'm thinking it should still allow to pass in > >> some > >>> context object (we may define an interface for it, with "protocol" > being > >> a > >>> mandatory field) to be able to continue using it in the future as well > as > >>> to let user explicitly specify that they want stream id for http2 or > >> http2 > >>> for example? > >>> Thanks, > >>> Sudheer > >>> On Monday, August 31, 2020, 06:49:57 PM PDT, Masakazu Kitajo < > >>> m4s...@gmail.com> wrote: > >>> > >>> Sudheer, > >>> > >>> Would a separated API to get the actual protocol active for the given > >>> request satisfy you? I don't understand how indicating the actual > >> protocol > >>> helps something. > >>> > >>> I think I understand your concern. Future protocols may need different > >>> params, or might have different representations. However, H/2 is > >> extensible > >>> so any APIs that have "Http2" in their names still can be incompatible > if > >>> we support extensions. The proposal for a new priority scheme is a good > >>> example. > >>> > >>> In that sense, passing a context object may be helpful, and more > flexible > >>> than having fixed context in API names, but maybe it's too early to > think > >>> about it. > >>> > >>> Masakazu > >>> > >>> > >>> On Mon, Aug 31, 2020 at 11:05 AM Sudheer Vinukonda > >>> <sudheervinuko...@yahoo.com.invalid> wrote: > >>> > >>>> Hmm, it does sound appealing to avoid having to duplicate API for > >> common > >>>> attributes between H/2 and H/3, but, I'm thinking the API should still > >>>> somehow explicitly indicate (either via an input param or an output > >>> param) > >>>> the actual protocol active for the given request. It seems a bit too > >>>> confusing (and potentially problematic depending on how things evolve > >> in > >>>> the future) to hide that aspect. > >>>> Thoughts? > >>>> > >>>> > >>>> On Sunday, August 30, 2020, 06:56:49 PM PDT, Masakazu Kitajo < > >>>> mas...@apache.org> wrote: > >>>> > >>>> And there is a proposal for a new priority scheme that may be used for > >>>> both > >>>> H2 and H3. > >>>> https://tools.ietf.org/html/draft-ietf-httpbis-priority-01 > >>>> > >>>> I haven't read it, but it would be nice if the TS API could be > >> compatible > >>>> with the new scheme. > >>>> > >>>> On Mon, Aug 31, 2020 at 10:48 AM Masakazu Kitajo <mas...@apache.org> > >>>> wrote: > >>>> > >>>>> I think we should avoid adding APIs just for HTTP/2 unless it's > >> really > >>>>> HTTP/2 specific. Are we going to add TSHttpTxnClientHttp3StreamIdGet > >>> and > >>>>> TSHttpTxnClientHttp3PriorityGet as well? > >>>>> > >>>>> I'd omit the "2" in the API names, and rephrase the API doc like: > >>>>> This API returns an error if the provided transaction does not have a > >>>>> stream id. > >>>>> > >>>>> Also, a stream id on HTTP/3 (QUIC) is 62 bit integer. > >>>>> > >>>>> Masakazu > >>>>> > >>>>> > >>>>> On Sat, Aug 29, 2020 at 5:24 AM SUSAN HINRICHS <shinr...@ieee.org> > >>>> wrote: > >>>>> > >>>>>> +1 > >>>>>> > >>>>>> On Fri, Aug 28, 2020 at 3:23 PM Sudheer Vinukonda > >>>>>> <sudheervinuko...@yahoo.com.invalid> wrote: > >>>>>> > >>>>>>> +1. > >>>>>>> Sounds reasonable to me. > >>>>>>> > >>>>>>> > >>>>>>> On Friday, August 28, 2020, 07:41:18 AM PDT, Brian Neradt < > >>>>>>> brian.ner...@gmail.com> wrote: > >>>>>>> > >>>>>>> Hi dev@trafficserver.apache.org, > >>>>>>> > >>>>>>> It would be helpful to add HTTP/2 stream and priority support to > >>>> Traffic > >>>>>>> Dump. In order for the plugin to access this information, I > >> propose > >>>>>> adding > >>>>>>> the following two functions to the TS API: > >>>>>>> > >>>>>>> tsapi TSReturnCode TSHttpTxnClientHttp2StreamIdGet(TSHttpTxn txnp, > >>>>>> uint32_t > >>>>>>> *stream_id); > >>>>>>> > >>>>>>> tsapi TSReturnCode TSHttpTxnClientHttp2PriorityGet(TSHttpTxn txnp, > >>> int > >>>>>>> *stream_dependency, int *weight); > >>>>>>> > >>>>>>> I've created a draft PR with this implemented along with Traffic > >>>> Dump's > >>>>>> use > >>>>>>> of it: > >>>>>>> https://github.com/apache/trafficserver/pull/7149 > >>>>>>> > >>>>>>> Please let me know if there are any concerns or suggestions for > >>>>>> improvement > >>>>>>> concerning this. > >>>>>>> > >>>>>>> Thanks! > >>>>>>> Brian > >>>>>>> > >>>>>>> -- > >>>>>>> "Come to Me, all who are weary and heavy-laden, and I will > >>>>>>> give you rest. Take My yoke upon you and learn from Me, for > >>>>>>> I am gentle and humble in heart, and you will find rest for > >>>>>>> your souls. For My yoke is easy and My burden is light." > >>>>>>> > >>>>>>> ~ Matthew 11:28-30 > >>>>>>> > >>>>>> > >>>>> > >>>> > >>> > >> > >> > >> > >> -- > >> "Come to Me, all who are weary and heavy-laden, and I will > >> give you rest. Take My yoke upon you and learn from Me, for > >> I am gentle and humble in heart, and you will find rest for > >> your souls. For My yoke is easy and My burden is light." > >> > >> ~ Matthew 11:28-30 > >> > -- "Come to Me, all who are weary and heavy-laden, and I will give you rest. Take My yoke upon you and learn from Me, for I am gentle and humble in heart, and you will find rest for your souls. For My yoke is easy and My burden is light." ~ Matthew 11:28-30
