On Oct 13, 2013, at 2:15 PM, Uri Shachar <[email protected]> wrote:
> > > > Thanks for the review James. > I agree with most of your points -- with a couple of exceptions: > > On Wed, 9 Oct 2013 10:41:42 -0700 James Peach wrote: >> On Oct 8, 2013, at 1:19 PM, [email protected] wrote: >> >>> Updated Branches: >>> refs/heads/master 7ba121c9a -> 3c0c835c1 >>> >>> >>> TS-2268 Add support for opening protocol traffic sockets through the >>> traffic_manager. >> >> Hi Uri, >> >> I don't much like this patch :) From the JIRA ticket, it is intended to (1) >> allow traffic_manager to hold ports for plugins and (2) support the full >> port specification syntax. >> >> (2) is already supported by the TSPortDescriptor API, which actually >> supports the requirement better than this implementation does. >> >> I can see how (1) is interesting, but I don't think that this is the right >> API. This implementation looks a lot like it supports one specific use case >> (not sure what the exact requirement it), but would be difficult to use more >> generally. > > There's also a small (3) - create a single point of configuration for all ATS > ports. I can see that it's nice to have the ports in config. I don't have a strong opinion on whether it really needs to be all on the same line. >> - How do multiple plugins use this API? > > I fully agree (and was planning to add support for this) - You're right that > without this the API is probably too specific. > >> - How do plugins use this to accept on SSL sockets? > > In the current implementation - they don't. There are a couple of issues to > consider here: > 1) How do we allow protocol ordering (what if I want a protocol plugin to > handle the connection _before_ the SSL engine) What's the use case for this? A plugin that implements it's own TLS? Is that needed? > 2) Protocol Plugin chaining implementation Again, what is the use case? I'm not sure that protocol plugin chaining would look like ... > > >> - We already have 3 *Accept() APIs, why can't they support this >> requirement? > > Isn't it worthwhile to have a way to specify that we require the port to be > pre-opened? The other option I can see (without adding API like > TSPortDescriptorRendevous) would be to pass a flag.... I'm not sure that it's worthwhile. If it's protecting against traffic_server crashes, that seems nice to have, but maybe not very important. Is that the only use case? > I don't see a problem with creating multiple variants (in the API layer only) > to emphasise the difference in behaviour. No, I think it's fine if it's necessary. But it's preferable to have a smaller API surface so that developers can reuse their knowledge and have fewer APIs to learn. We also have an easier time documenting and teaching a smaller API set. > >> - The name breaks API conventions (ie. there's no such object as a >> TSPluginDescriptor) > > Agreed - TSNamedAccept(STRING) maybe.... > >> I think that a more generally useful facility would be to consider extending >> the TSPortDescriptor to handle requirement (2). The trick for this will be >> that traffic_manager has to open the sockets and listen(), but not ever >> accept(). This means that we need to communicate the descriptor string to >> traffic_manager. Here are some suggestions on how we could do that: >> >> a) Add a owner=STRING tag to the socket descriptor. This could cause >> traffic_manager to listen but not accept. A subsequent call to a new API >> TSPortDescriptorRendevous(STRING) would return a corresponding >> TSPortDescriptor that can be used with TSPortDescriptorAccept(). > > In progress on the first half - I dislike TSPortDescriptorRendevous for two > reasons: > 1) 99.9% of uses would call TSPortDescriptorRendevous and then > TSPortDescriptorAccept Yep, but that's trivial to do. > 2) Multiple ports with the same owner tag will make the API a bit more > complicated since we'll need to return a list Good point. That's a good argument for not bouncing through a TSPortDescriptor object. J
