On Thu, Apr 3, 2014 at 8:20 AM, James Peach <jpe...@apache.org> wrote:
> 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.
>
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
