Thanks for the comments. I'll circle back with the team and will revise. I have to check, but I actually think the first few versions of this (when it was individual) *may* be pre-5378, but will double check.
On Fri, Aug 14, 2015 at 6:33 PM, Alissa Cooper <[email protected]> wrote: > I have reviewed this document in preparation for IETF last call. Before > proceeding to last call, I have some comments and questions that I’d like > to discuss. I’ve also included a list of nits that should be resolved > together with any last call comments. > > > == Substantive comments and questions == > > = General > > I think this document should be listed as updating RFC 6940 in the > document header. > > The document contains a disclaimer for pre-RFC5378 work, which I assume > is in error. If using xml2rfc to generate the text, using the most > up-to-date version of xml2rfc to create the next version should fix this. > > = Section 4.3: > > "There have been proposals that RouteQuery and a series of Fetch > > requests can be used to replace the PathTrack mechanism, but in the > presence of churn such an operation would not, strictly speaking, > provide identical results, as the path may change between RouteQuery > and Fetch operations. (although obviously the path could change > between steps of PathTrack as well)." > > > This text is confusing. It doesn’t explain why PathTrack is being defined > rather than peers using RouteQuery plus Fetch. Furthermore, the document > does not explain how peers are supposed to interpret the results of the > PathTrack method in the event that the route does change in between steps. > This seems like a substantial omission. > > In general, this document provides very little information about how peers > are expected to interpret and use the diagnostic information. I understand > that peers can do whatever they want with it, but in several places the > fact that this is not explained makes the reasoning behind the protocol > design decisions opaque. I would suggest providing a little more context, > at the very least by moving the examples into the body of the draft > somewhere. > > = Section 4.4: > > I’m a little confused about the values given for the error codes. Do you > want IANA to chose these specific values? Or are they just there as > placeholders? The customary way of doing placeholders would be as follows: > > Code Value Error Code Name > [TBD1] Underlay Destination Unreachable > [TBD2] Underlay Time exceeded > [TBD3] Message Expired > [TBD4] Upstream Misrouting > [TBD5] Loop detected > [TBD6] TTL hops exceeded > > > These placeholders would then get used throughout the document. > > This section also says: > > "In addition, this document introduces several types of error information > in the error_info field in the case of Code 0x65. ... Here are some > examples for the error info. ... The error_info field values of the Code > 0x66 to 0x70 are to be application specific and defined by the particular > overlay." > > I’m not sure what is meant by all of this text. As defined in RFC 6940, > error_info is an optional implementation-specific byte string. If you are > intending to require implementations to include specific text in the > error_info for the first error code, then you need to explain under what > conditions the specific text needs to be included for each string. And the > text about the other codes seems redundant with how error_info is already > defined in RFC 6940. > > = Section 5.2: > > "ext_length : the length of the returned DiagnosticInfo information > > in bytes. If the value is greater than or equal to 1, then some > extended diagnostic information is requested." > > > This is defining the diagnostic response — why would the extension be > requesting something? And if it is possible to use an extension in that > way, the expected behavior from the initiator when it receives such an > extension needs to be described, or at least it needs to be noted that a > diagnostic response could provoke a request back to the initiator. > > Also, in the struct the diagnostic info is called “diagnostic_info_contents” > but in the text it is called “diagnostic_information.” These should be > the same I think. > > = Section 5.3: > > I’m concerned about the way STATUS_INFO is defined. First, why are there > 16 different levels of congestion status? What are they supposed to > signify? If there is no standardized way of measuring congestion defined, > how is a node supposed to interpret STATUS_INFO received from different > peers? That is, couldn't one peer’s level 7 actually mean it is less > congested than another peer’s level 3, and won’t that make the information > somewhat meaningless to the initiator? Maybe this would be clearer if you > could explain what you expect an initiator to do with the information about > 16 different levels of congestion. > > SOFTWARE_VERSION doesn’t really seem like diagnostic information. While > all of the other fields may be constantly changing and therefore it would > make sense to inquire about them in a diagnostic fashion, SOFTWARE_VERSION > seems more like data that nodes could register or exchange when they join > the overlay. Why is it included as a diagnostic? > > Also, doesn’t the SOFTWARE_VERSION type need a length or a specified > delimiter that indicates the end of the string? Otherwise, how is the > recipient supposed to know when to stop parsing it? > > The definitions of EWMA_BYTES_SENT and EWMA_BYTES_RCVD seem problematic. > > sent = alpha x sent_present + (1 - alpha) x sent > > rcvd = alpha x rcvd_present + (1 - alpha) x rcvd > > > As written these equations are not right because sent/rcvd appear on both > sides. It would be clearer to use last_sent and last_rcvd or some such on > the right-hand side of these equations. But this begs some bigger questions: > > - Does this place a requirement on all nodes implementing this > specification to have to calculate these values every 5 seconds? > - How are the values calculated the first time? > - How was the value of 5 seconds chosen? > > Finally, I have trouble understanding why only some of the types are > subject to access control as described in Section 7. It seems to me that in > some situations all of the fields defined here could be considered > sensitive by certain nodes or on certain networks. What is the > justification for only making some of them subject to access control? > > = Section 6.1: > > “The destination field MUST be set to the desired destination, which MAY be > either a NodeId or ResourceID but SHOULD NOT be the broadcast NodeID." > > What is the expected behavior if a Ping or a PathTrack does get sent to > the broadcast NodeID? Or should these requirements really be MUST NOTs? > > = Section 6.4: > > "However, for a single hop > measurement, the traditional measurement methods MUST be used instead > of the overlay layer diagnostics methods." > > > What is meant by the traditional measurement methods? > > = Section 9: > > Please only use either RFC-XXXX or RFC-AAAA but not both as placeholders. > > Please also use [TBDX] for values you expect to be allocated by IANA. > > > == Nits == > > = General > > The document seems to repeatedly explain that it is doing things “as > specified in RFC6940.” I don’t think this necessary in many cases. It’s > worth doing a pass through and removing the places where it’s obvious that > this spec is building on RFC 6940. > > = Section 4.1: > > OLD > > The extensions strictly follow RELOAD > specification on the messages routing, transport, NAT traversal etc. > > > NEW > > The extensions strictly follow how RELOAD specifies message routing, > transport, NAT traversal, and other RELOAD protocol features. > > > = Section 4.2.1: > > s/as specified in Table 4 and specified in the RELOAD/as specified in > Table 4/ > > s/new requests of the the/new requests of the/ > > = Section 4.3: > > OLD > > We define a simple PathTrack method for retrieving diagnostic > information iteratively. The mechanism defined in this document > follows the RELOAD specification, the new request and response > message use the message format specified in RELOAD messages. Please > refer to the RELOAD [RFC6940 <https://tools.ietf.org/html/rfc6940>] for > details of the protocol. > > The operation of this request is shown below in Figure 2. > > > > NEW > > We define a simple PathTrack method for retrieving diagnostic > information iteratively. The operation of this request is shown below in > Figure 2. > > > = Section 4.3.1.2: > > s/after that/after receiving a PathTrackAns where the next_hop node ID > equals the responding node ID/ > > = Section 4.3.2.1: > > s/PathTrack Response/PathTrack response/ > > = Section 4.4: > > s/code. and we define/code. We define/ > > = Section 5: > > s/Path_track methods/PathTrack methods/ > > = Section 5.1: > > s/Note that is NOT/Note that it is not/ > > = Section 5.2: > > s/Note that is NOT/Note that it is not/ > > s/consists one or more/consists of one or more/ > > = Section 6.1: > > s/`MAY/MAY/ > > = Section 6.2: > > s/return the response the initiator node/return the response to the > initiator node/ > > s/The peer SHOULD/The intermediate peer SHOULD/ > > > _______________________________________________ > P2PSIP mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/p2psip > >
_______________________________________________ P2PSIP mailing list [email protected] https://www.ietf.org/mailman/listinfo/p2psip
