[ I don't think this is worth time at today's interim. These should generally be quite quick to address. ]
Hello TAPS! A few interims ago I volunteered to review draft-ietf-taps-arch. I'm obviously biased in everything TAPS, yet I'm not an author of this particular draft, nor have I read it in a long time... After volunteering, I expected to receive an email pinging me about it. I never did, and now I think that I shouldn't have waited for an email - I believe that this was a misunderstanding. Be that as it may, now I have finally reviewed it :) This review concerns the "Editor's copy" version at https://ietf-tapswg.github.io/api-drafts/draft-ietf-taps-arch.html as of 11:51 (my time) on 19. 10. I believe that it includes the latest PR from Gorry, and it probably matches the latest version at the time of writing this. I'll just put all my comments here in sequence of appearance in the document instead of grouping them and opening issues, and leave it to the document authors to decide what's a nit and what's a content change that warrants further discussion - I trust you to "do the right thing" (tm) (*truly* a trademark, though not mine :-) ). Without further ado, here come my comments: Introduction paragraph 2: here we have 3 occurrences of "provide". I would replace the middle one - e.g.: "...reusable architecture that offers a common interface ..." Section 1.2, item 3, starting with "Section 4 presents...": I believe that the last ( = second) sentence of this item must have ended here by error, it seems out of place ("The Preconnection allows applications..."), and I think it can safely be removed. About the last sentence in item 4, see: https://github.com/ietf-tapswg/api-drafts/issues/939 Section 2, right below Figure 2: "The Transport Services API [I-D.ietf-taps-interface] defines the interface for an application to create network connections and transfer data." => Should this be "Connections" instead of "network connections" ? Section 2.1, paragraph 2, last line: "... made explicit in sockets." seems strange. Should this be "in the sockets API." ? Section 2.2 paragraph 1, this sentence: "While TCP does indeed provide the ability to send and receive data as streams, most applications need to interpret structure within these streams." Two issues here. First, I would switch to singular because TCP is not a multi-streaming protocol. I.e., change this to: "While TCP does indeed provide the ability to send and receive data as a stream, most applications need to interpret structure within this stream." Second, is "stream" a known concept? I don't think it's defined in this document, and I'm a little unsure because, even when using singular, someone might read "stream" and think of "one stream out of several, in a multi-streaming protocol". Maybe this is nit-picking, but if you agree, I would prefer to replace "stream" with "data stream", or having a definition of "stream" and capitalizing it (but then the definition would have to appear before the first use of this term...) Next paragraph: double "provide" in: "Providing a message-based abstraction provides many benefits, such as:" - I suggest to replace the second with "has". In the following item list, I would remove the third item: "the ability to manage dependencies between messages, ..." While it is true that a message-based abstraction enables this, a reader of this document might get the impression that the TAPS system defined in the interface draft offers this specific functionality. It really doesn't, it would still have to be implemented on top of it. For this reason, this item strikes me as a bit misleading, and I think it should be removed. Section 3, paragraph 2: "it can provide access to multiple sets of protocols and protocol features" - remove "sets of", this adds an unnecessary level of indirection here. I guess the transport property profiles in interface, section B.2 would be such sets, but still... Last paragraph before section 3.2: "Applications using a Transport Services system interface are REQUIRED to be robust to the automated selection provided by the system." - why say "system interface" here all of a sudden? This gives an impression as if there were two things: the API, and then perhaps also a separate "system interface". I'd rather replace this with "API". Last paragraph of section 3.4: "In normal use, a Transport Services system SHOULD result in consistent protocol and interface selection decisions for the same network conditions given the same set of Properties." Isn't this too restrictive? It prohibits a Transport Services system from improving. I would change this to: "In normal use, a given version of a Transport Services system SHOULD result in consistent protocol and interface selection decisions for the same network conditions given the same set of Properties." Section 4.1.5: "For example, a Message can be marked with a Message Property indicating that it is the final message on a connection if the peer sent a TCP FIN." I would stop this sentence after "on a connection." - or is it just me misunderstanding something? I thought (and -interface section 9.1.3.5 seems to agree with me) that the idea of "final message for TCP" really is that setting the property means that THIS message that I'm sending gets a FIN flag. Why would a FIN from the peer make me set this property? FWIW I could go on sending more data, and use this property much later? Section 4.1.7, item 1: "end of stream in the ..." - see above for a discussion of the word "stream". Section 4.2, item 5: "One Protocol Stack that can be used by an application for a connection, of which there can be several." => I believe it should be "Connection" here, not "connection". Section 4.2.2, first sentence: "or some combination of all of these" - should this be "or using some combination of all of these" ? Section 4.2.3, item 3: "Performance concerns about Connections introducing head-of-line blocking due to multiplexing or needing to share state on a single thread." I don't think application programmers should decide against using functions such as Connection Grouping because they try to optimize their performance in this way. If it performs poorly, the Transport Services system shouldn't do it. I know this isn't always black and white... I'm guessing that this was written with HTTP/2 in mind. Still ... if a mechanism is judged as generally performing better most of the time, the system should probably use it. If it doesn't perform better e.g. depending on the length of transfers, which may be known from HTTP header, then maybe the Transport Services system is the ideal place to dynamically make such a decision? Anyway - I don't think that we should indirectly suggest to application writers that internal functions chosen by the Transport Services system on the basis of high-level information like "these Connections belong together" might in fact perform poorly, and hence they may be better off not using them if they want the best performance for their application. So, I would remove this item. I hope this is useful ! Cheers, Michael _______________________________________________ Taps mailing list Taps@ietf.org https://www.ietf.org/mailman/listinfo/taps
