You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by James Peach <jp...@apache.org> on 2014/04/03 02:20:03 UTC
Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack()
API
On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yu...@gmail.com> wrote:
> On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jp...@apache.org> wrote:
>
>> On Mar 13, 2014, at 2:56 PM, James Peach <jp...@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 <qi...@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
Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack()
API
Posted by Yunkai Zhang <yu...@gmail.com>.
On Thu, Apr 3, 2014 at 8:20 AM, James Peach <jp...@apache.org> wrote:
> On Mar 27, 2014, at 8:35 PM, Yunkai Zhang <yu...@gmail.com> wrote:
>
> > On Fri, Mar 28, 2014 at 8:00 AM, James Peach <jp...@apache.org> wrote:
> >
> >> On Mar 13, 2014, at 2:56 PM, James Peach <jp...@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 <qi...@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.
>
Oh, I think I know what you worry about. Yes, we need a comm solution(Or
API) to make the above APIs keeps consistent with proto_stack.
Let me dig more for it.
>
> I guess TSNetConnect does not get logged anyway, right?
>
Yes, but I think TSNetConnect() is different with TSHttpConnect(), it does
not depend on PluginVCCore, the caller of it is a pure client, but the
caller of TSHttpConnect() is a intermediary.
>
> >> 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?
>
Good idea!
>
> J
>
--
Yunkai Zhang
Work at Taobao