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/03/13 22:56:47 UTC
Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack()
API
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 :)
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
Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack()
API
Posted by James Peach <jp...@apache.org>.
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 James Peach <jp...@apache.org>.
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 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.
>
> 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.
>
> Would "WS" be a more conventional abbreviation for WebSockets? It took me
> a while to figure out "WBSK" :)
>
OK to me:)
>
> J
>
--
Yunkai Zhang
Work at Taobao
Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack()
API
Posted by James Peach <jp...@apache.org>.
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?
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.
Would "WS" be a more conventional abbreviation for WebSockets? It took me a while to figure out "WBSK" :)
J
Re: [4/5] git commit: TS-2612: Indroduce TSHttpConnectWithProtoStack()
API
Posted by James Peach <jp...@apache.org>.
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?
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.
Would "WS" be a more conventional abbreviation for WebSockets? It took me a while to figure out "WBSK" :)
J