On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yunkai...@gmail.com> wrote:
> On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jpe...@apache.org> wrote:
>
>> On Mar 13, 2014, at 2:56 PM, James Peach <jpe...@apache.org> wrote:
>>
>>> On Mar 11, 2014, at 8:12 PM, yun...@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....@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
