Re: [Gen-art] Gen-ART: Re-review of draft-ietf-p2psip-base-23

[Gonzalo Camarillo <[e-mail]>]

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.
>