These all look reasonable, and most easy to resolve. Now as github issues:-)
Gorry
On 20/10/2021 15:00, Aaron Falk wrote:
Hi Michael-
Thanks for the review. Glad to see the doc hasn’t ‘aged’ much as the
others have been refined. Let’s assume I dropped the ball on reminding
you about the review. :)
--aaron
On 20 Oct 2021, at 9:31, Michael Welzl wrote:
[ 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
<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
<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
<https://www.ietf.org/mailman/listinfo/taps>
_______________________________________________
Taps mailing list
Taps@ietf.org
https://www.ietf.org/mailman/listinfo/taps
_______________________________________________
Taps mailing list
Taps@ietf.org
https://www.ietf.org/mailman/listinfo/taps
