Hi Mary, thanks for your review. I am adding Dean to this thread since he is currently editing the document.
Cheers, Gonzalo On 14/12/2012 8:02 PM, Mary Barnes wrote: > Russ asked that I re-review the updated document. > > I am the assigned Gen-ART reviewer for this draft. For background on > Gen-ART, please see the FAQ at > <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>. > > Document: draft-ietf-p2psip-base-23 > Reviewer: Mary Barnes > Review Date: 14 December 2012 > Re-Review started Date (-21): 28 March 2012 (was told a new version > was coming out) > Original Review Date (-17) : 6 August 2011 > IETF LC End Date: 22 July 2011 > > Summary: Not Ready. > > I reviewed through section 6 of this version (section 5 of -17) > against my comments on the -17 . Since many of my comments had not > been addressed, I'm not going to take the time to re-review the > remainder of the document. > > I also *strongly* recommend that you ensure Henning has reviewed this > document *before* it gets into the RFC editor's queue. He has fairly > strict (and quite accurate) views on grammar and structure but it > really isn't good to have so many changes go in at AUTH48 as there is > a risk of introducing true technical bugs or changing something that > was carefully crafted to achieve WG consensus: > http://www.cs.columbia.edu/~hgs/etc/writing-bugs.html > > > Comments: > ========= > > I have reviewed my previous comments on this document -17 as compared > to the -23. Comments from the -17 that remain are prefaced with [-17] > > Major: > ------ > > General: > --[-17] The message names are used inconsistently. The IANA registry > has the message codes all lower case. There are places in the > examples and the body of the document where the _req part is missing, > there is no "_", the first letters are Upper case, etc. The messages > are also used in a general sense - e.g., "MUST perform an Attach", > which should really be stated as "MUST send an attach_req message). > [-23] For the latter point, Cullen responded that this isn't really > just talking about sending a message but rather performing a function > that results in a message being sent. > > However, I still contend that there is inconsistency and the whole > concept of an Attach function/process/whatever is not well defined. > There are times when "attached" appears to be used purely as a verb > rather than referring to the functionality, but it is not always > clear. For example, the last two sentences in section 3.4: > "Instead, we use the Attach request to establish a > connection. Attach uses ICE [RFC5245] to establish the connection. > It is assumed that the reader is familiar with ICE." > I will note that the only mention of "Attach" previously was in the > definitions for Connection Table ("Attach handshakes") and Routing > Table ("some peers will have Attached"). The word "Attach" is used > with the _req or the word "request" in places like the following in > section 3.4 (1st sentence): "There are two cases where Attach is not > used." I have no idea really whether this means this Attach > function/process is not needed or the message is not required to be > sent. > > Major - detailed: > > - Section 3.4, last paragraph: Accepting Cullen's comment that this > text is not normative, I'll let my first set of -17 comments on this > one go. However, the following is still not clear to me: > What is meant by the "specified link set" - is that referring to all > the nearby peers or the peers+enough others? Or is this more clearly > (and normatively) specified elsewhere, such that a reference could be > added. > > - [-17 5.1.2] Section 6.1.2: > -- Last paragraph (9) mixes normative text with in an example. The > normative text needs to be separated from the example - e.g., split > some of the sentences into the normative behavior and then the > example. The example should be written in terms of what happens (which > is what was done in the previous examples in this section and not what > SHOULD, MAY or MUST happen). For example, this sentence: > "Node A MUST implement an algorithm that ensures that > A returns a response to this request to node B with the destination > list [B, Z, Y, X], provided that the node to which A forwards the > request follows the same contract. > should have a generic normative statement rather than saying "Node A > MUST". It should be something like: "A node must implement an > algorithm that ensures it returns a response …", then the same > sentence can be a "For example, Node A MUST…" > > - [-17 5.3.2.1] Section 6.3.2.1: > --1st paragraph: shouldn't this be normative? > "the configuration document has a sequence number..." -> "the > configuration document MUST contain a sequence number which MUST be > monotonically increasing mod 65535" > > - [-17 5.3.2.2] -Section 6.3.2.2: > --Paragraph after second structure, suggest the following changes: > ---"the generation counters should" -> "the generation counters SHOULD" > ---"An implementation could use.." -> "An implementation MAY use…" > ---"the node confirms that the generation counter matches" -> "the > node MUST confirm that the generation counter matches" > ---"If it does not match, it is silently dropped." -> "If it does not > match, it MUST be silently dropped." > > - [-17 5.3.3.1] Section 6.3.3.1: > --1st para: "a request returns" -> "a request MUST return" > --1st para: "then the message code is the response code that matches > the request" -> > "then the message code MUST be set to the response code that matches > the request" > --2nd para: "message code is set" -> "message code MUST be set" > > - [-17 5.3.4] Section 6.3.4 - needs normative language: > --1st paragraph after GenericCertificate structure: > ---"All signatures are formatted" -> "Alls signatures MUST be formatted" > ---"The input structure…varies.." -> "The input structure … MAY vary…" > > - [-23] Review stops at section 6.4.2 > > > Minor: > ------ > - idnits identifies 5 errors (downrefs). There are also unused > references and 22 warnings that should be reviewed - the ones, in > particular, which should be fixed are with regards to normative > language usage. > > - While Cullen responded to this comment with an explanation, there > was no change to clarify the text: > - [-17] Section 1.2.1, 2nd paragraph: I don't understand the example > as to why a single application requires multiple usages - i.e, why > voicemail? Isn't the intent to say that an application might need to > use both SIP and XMPP - i.e., you wouldn't define a "usage" for an > application, would you? > > - [-17] Section 2. Some of the definitions use terms that are not yet > defined, so that's not particularly helpful - i.e,. Attach and Update > are used. In some cases (e.g., Connection Table), you could just > delete the statement with the reference. > > - [-17] Section 3.1, 1st para: This sentence doesn't grok because the > concept of "overlay parameters" hasn't been mentioned before this > section nor has the concept of a "configuration document". > "The overlay parameters are specified in a configuration document." > At a minimum those terms should be defined in the terminology section > and introduced in the intro paragraph in section 3. > > - [-17] Section 3.3, last paragraph. Add a reference to 5.4.2.4 > after "RouteQuery method" > > - [-17] Section 5.1.2: > --1st paragraph: I have no idea what the "other three cases" references. > > - [-17 Section 5.3] Section 6.3: The three parts of the messages are > listed and the last sentence says that "The following sections > describe the format of each part of the message", but there are 4 > sub-sections in 5.3, with the first section NOT describing one of the > three parts of the messages. I suggest to strike that last sentence > and just put references to the sections in the list items for each of > the three messages > > - [-17 Section 5.6] Section 6.6: > -- The last three paragraphs are confusing. > ---In particular, it's not clear what algorithms are being referenced > in this sentence: > "We will first define each of these algorithms, then > define overlay link protocols that use them." > --- In the note paragraph, I believe "These protocols have been > chosen…" is referring to the three overlay link protocols supported by > RELOAD per this document. > ---The Note to implementors paragraph seems out of context > --- Per my next comment, I suggest to move the detailed discussion of > future overlay link protocols to the appendix, so some of this text > may be relevant only to that. > > -[ -17 5.6.1] Section 6.6.1: I suggest that the sub-sections in this > section be moved to an appendix and a reference to such be added. > > [-23] Did not review beyond section 6. > > Nits: > ----- > > - [-17] Section 2. > --General: It would be helpful if the terms were alphabetized. > [Note, Cullen's response was that this causes forward reference > problems, but that's unavoidable. If the terms are alphabetized, it is > much easier to find them. ] > --Node: "We use the term "Node" to refer to …" -> "Refers to …" or > something to make it more consistent with the format for the other > terms. > > - Section 6.1.1, last bullet: "later" -> "latter" > > - Section 6.1.3: Shouldn't this be "For example," rather than "I.e.,"? > If "I.e.," is appropriate in that those are the only routing > algorithms which might be used, then I would suggest adjusting the > sentence structure by using words rather than the abbreviation (it's > debatable whether the latter is grammatically correct) to something > like. "This means that a node…". > > - [-17 5.5.1.6] Section 6.5.1.6: Add references for SCTP and DCCP. > > [-23] Did not review beyond nits identified for section 6. > _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
