> On 17 Sep 2021, at 22:57, Robert Sparks via Datatracker <[email protected]> > wrote: > > Reviewer: Robert Sparks > Review result: On the Right Track > > This is an art-art early review of draft-ietf-taps-interface > > This document reads fairly well. I have some structural concerns (see the > early > review of the architecture document). In particular I think this document > contains the actual definition of the architecture and some of that is > inferential. > > There are several places where the language in this document, once published > as > an RFC, will not age well. Search for occurrences of the word "modern" and > consider writing less to motivate and more to define. Reconsider the use of > terms like "traditional". > > Reconsider words like "atop" and "underneath" when talking about interface > boundaries. (Also, I don't know what it means to be atop an architecture.) > > I cringe at systems that define Integers to be integers and some other > enumerated things ("special values"). Is it necessary to define things this > way? I understand it's currently typical to have this behavior in the APIs of > the protocols this system is attempting to abstract, but is it impossible to > stop propagating that kind of type confusion? This API could move the things > special values are being used for into separate arguments for example. > > The definition of Numeric in 1.1 is circular. > > The concept of cloning is used in the overview without introduction. I've > suggested an earlier discussion of it in the architecture document, which > would > help, but even in this document, there should be an introduction or forward > reference. > > As you can tell, I don't think the documents do what the first sentence of the > API summary claims. This document would not lose anything if you deleted the > sentence. > > At the end of the API summary, there's a paragraph that looks like a > "Structure > of this document" discussion - should it be in a different place? The list in > the paragraph is odd - why is Section 8, for instance, not in the list? > > Consider showing an explicit context in the Send in the example in 3.1.1 - it > would make the reader guess less to wait to introduce implicit contexts later. > > In the text version of the document, the last paragraph before 3.1.3 has some > markup (~~~) that has leaked through. > > 3.1.3 has a comment that's too long for the text version > > The first sentence-paragraph of section 4 is complex. Please break it apart. > > At the discussion of Transport Property Names at 4.1, I think you need to be > more specific than "alphanumeric". Explicitly list what characters are > allowed. > > The "reformatted where necessary" part of the requirement in the last > paragraph > of 4.1 makes the MUST NOT almost impossible to conform to. I think I > understand > what the paragraph is trying to say, but the current formulation leaves the > reader guessing. I think you are trying to say "Avoid using any of the terms > listed as keywords in the protocol numbers registry as any part of a vendor or > implementation-specific property name". > > Do you really mean "host" at "(endpoints) SHOULD represent the same host" in > section 6, or do you mean something more like service, given that an endpoint > may configured with a DNS name? > > I think there's a need for more discussion and clarity around the restriction > that "An Endpoint cannot have multiple identifiers of a same type set." Why > not? You effectively are letting the endpoint have (potentially) multiple IP > addresses when you create it with an identifier that is a DNS name. This feels > like an artificial, inconsistent, restriction given the rest of the document > (look at the 3rd paragraph of 6.1). > > Would there be any benefit of having something like "WithServiceName" to > qualify a specifier for use with NAPTR/SRV? Or is the thinking that the > resolution of the thing handed down through WithHostname is restricted to > A/AAAA? > > Why does the API have both the ability to say LocalSpecifier.WithInterface and > to set interface requirements on TransportProperties? Why not just keep the > latter and remove the former? > > I would really like to see an example of how an application that relies on ICE > learns about new candidates as they are discovered - the callback pattern is > not clear to me yet. > > There are several places where the document says "Changes to a <foo> after > func() has been called do not have any connection on existing <bar>s". This > feels like an opportunity for more clarity about what a <foo> really is. The > document uses the word Object, and maybe there's an assumption that there's a > shared understanding of what the properties of an Object are. Being explicit > with something like "An Object is a collection of properties that > implementations of the API read and use..." to avoid the "here's a chunk of > memory that can get passed into the API that it can operate on, and expect the > caller to see the effects of the operation" interpretation. I _think_ what the > current text is trying to say is that the user of the API should assume the > implementation of the API made its own private copy of the Object at the point > of the given func() call and that it will not see any changes to the copy the > user has at any later point. But then there's some text that looks like "Once > a > <foo> has been used with a func(), it is invalid to modify any of its > properties." So... > > I'll have to read the document yet another time, perhaps, but after the few > times I went through it for this review, I'm still not clear on how/whether > connections are grouped because of actions from the remote end(s). If so, how > does the user learn about the grouping? Does it have to poll > Connection.GroupedConnections() every time its told of a new Connection? > > In section 8's third paragraph, the MUST NOT is not a good use of 2119/8174. > Are you trying to say protocol specific property _names_ must not be reused > across different protocols? > > At the last bullet before section 8.1, are you RFC7556 being pointed to as an > example way one might derive properties? If so, adjust the language to make it > clear that its an example. > > In 8.1.1 the second sentence is convoluted. Is there a way to restate it > without as many nots, nons, an different zeros? > > At 8.1.2 you say lower values have higher priorities for connPrio. But later, > at msgPrio, higher values have higher priority, and in the examples where you > speak of the interaction of connPrio and msgPrio, you imply that higher means > higher for _both_ properties. Please check for consistency and clarify. > > Is the "scope" used for message contexts (9.1.1) ever anything but a framer? > If > not, could this section just say framer? > > At the discussion of Send Events, I worry that the API is closing a door on > allowing the api to channel information about what's happening because of a > Send that may be available from future transports. Maybe such things would be > communicated as events from Connection instead (like Connection->SoftError), > even if they happened to be Send specific?
I added this as a set of issues in github, where we can also discuss whether/how to resolve these: https://github.com/ietf-tapswg/api-drafts/issues Michael _______________________________________________ Taps mailing list [email protected] https://www.ietf.org/mailman/listinfo/taps
