trafficserver-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From James Peach <jpe...@apache.org>
Subject Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack() API
Date Thu, 03 Apr 2014 00:20:03 GMT
On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yunkai.me@gmail.com> wrote:

> On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jpeach@apache.org> wrote:
> 
>> On Mar 13, 2014, at 2:56 PM, James Peach <jpeach@apache.org> wrote:
>> 
>>> On Mar 11, 2014, at 8:12 PM, yunkai@apache.org wrote:
>>> 
>>>> TS-2612: Indroduce TSHttpConnectWithProtoStack() API
>>>> 
>>>> 1) Firstly, add a bitmask, *proto_stack*, in NetVConnection/HttpSM,
>>>> and set it properly.
>>>> 
>>>> 2) For some plugins that using TSHttpConnect() API to do request,
>>>> the Logging module can't know what protocol stack is used, so I
>>>> add a new API:
>>>> 
>>>> TSHttpConnectWithProtoStack(struct sockaddr const* addr,
>>>>                           TSClientProtoStack proto_stack);
>>>> 
>>>> After introducing TSHttpConnectWithProtoStack() API, TSHttpConnect() API
>>>> would be a special case of it:
>>>> 
>>>> TSVConn
>>>> TSHttpConnect(sockaddr const* addr)
>>>> {
>>>>  return TSHttpConnectWithProtoStack(addr, (1u << TS_PROTO_HTTP));
>>>> }
>>>> 
>>>> Signed-off-by: Yunkai Zhang <qiushu.zyk@taobao.com>
>>> 
>>> This needs API review since the final form of the API was not reviewed
>> on dev@. I'll try to review this next week. Everyone else who reviewed
>> the original proposal should also review :)
>> 
>> I like the name "TSClientProtoStack". I don't like that it is tied to
>> TSHttpConnect; since it is a property of the VConn, doesn't it make more
>> sense to be able to get and set it on a TSVConn?
>> 
> 
> It seems not easy to do that, before a new VConn returned by
> TSHttpConnect() API, the connection might have been established
> asynchronously. That is say, unexpected client proto stack whould be passed
> to HttpSM before it was set, and HttpSM will copy the unexpected value to
> its internal HttpSM->proto_stack variable -- IIUC, logging module should
> not read VConn->proto_stack directly as VConn might have been released in
> logging phase.

Pretty sure that this would affect TSHttpConnectTransparent, TSFetchUrl and TSFetchPages.
If that's the case, I guess we can lump this with the other issues for needing a new HTTP
request API.

I guess TSNetConnect does not get logged anyway, right?

>> I don't think that users should have to deal with the bitmask
>> representation directly. It would be better to separate this into 2 types
>> (transport protocol and application protocol), and then do the bitshifting
>> internally. We should have internal helper functions to unpack the
>> bitfields.
>> 
> 
> I just worry about that there may exist some clients contain more than 2
> types in its proto stack in the future.

TSClientProtoStack TSClientProtoStackCreate(TSProtoType, ...)

// Websockets over SPDY over TLS ...
protostack = TSClientProtoStackCreate(TS_PROTO_TCP, TS_PROTO_TLS, TS_PROTO_SPDY, TS_PROTO_WBSK,
TS_PROTO_MAX);

What do you think?

J

Mime
View raw message