Subject: Re: [apps-discuss] [APPS-REVIEW] review of draft-ietf-p2psip-diagnostics-13

From: Carlos Jesús Bernardos Cano <[email-masked]>
Date: Mon, 02 Jun 2014 18:13:54 +0200

Hi Martin, authors, all,

@Martin: Thank you very much for your review.

@authors: please address Martin's comments and generate a new version of
the draft.

Thanks!

Carlos

On Mon, 2014-05-19 at 19:07 +0900, "Martin J. Dürst" wrote:
> I have been selected as the Applications Area Review Team reviewer for 
> this draft (for background on apps-review, please see 
> http://www.apps.ietf.org/content/applications-area-review-team).
> 
> Please resolve these comments along with any other Last Call comments 
> you may receive. Please wait for direction from your document shepherd 
> or AD before posting a new version of the draft.
> 
> Document: draft-ietf-p2psip-diagnostics
> Title: P2P Overlay Diagnostics
> Reviewer: Martin J. Dürst
> Review Date: various days before and including May 19, 2014
> IETF Last Call Date: not yet
> IESG Telechat Date: not yet
> 
> 
> Summary: To me, the draft looks in reasonable shape, with some issues to 
> be fixed as bellow. However, I'm not a SIP or P2P expert, and therefore 
> this review shouldn't be taken as an active endorsement of the draft.
> 
> 
> Preface: I sincerely apologize for the long time it took me to review 
> this draft. I have reviewed -13, but I have also checked the diff at 
> http://tools.ietf.org/rfcdiff?url2=draft-ietf-p2psip-diagnostics-14.txt 
> to make sure I haven't missed anything new.
> 
> 
> Overall issue: There were quite a few places in the text where the 
> wording choice was somewhat suboptimal. I strongly recommend a review by 
> a native English speaker familiar with the subject.
> 
> 
> Minor issues:
> 
> Structure: The meat of the draft is all in section 5. It may make sense 
> to split that section (but merge others) to get a better balance. But I 
> can't give a specific proposal, sorry. The start of section 5 also reads 
> like an abstract for a whole document.
> 
> p. 9: DiagnosticRequest has 'dmFlags' before 'length', but in the text, 
> the order is reversed. Please align.
> 
> p. 10: There is only a single 'kind' code (0xFFFE) reserved for private 
> use. Given that currently only about 16 such codes are defined in the 
> spec, it seems okay to reserve a somewhat larger number of these codes 
> for private/experimental use.
> 
> 5.1.2, timestamp_received (and timestamps in general): is a resolution 
> of milliseconds good enough? It may be way more than actually useful in 
> a world-wide overlay, but it could be less than desirable in a local 
> deployment. Same also for the limits (10 to 600 seconds) on the 
> expiration field. A local network should probably not need a timeout of 
> 10 seconds.
> 
> 5.1.3, PROCESS_POWER, BANDWIDTH: 32bits of MIPS and Kbps seem to cover 
> the current range of hardware, but what about extremely slow hardware or 
> very fast hardware in a few years? Similar for bandwidth. What about 
> e.g. defining some kind of log scale, which may reflect the long-term 
> realities of technology and market better? Same for MEMORY_FOOTPRINT.
> 5.1.3, SOFTWARE_VERSION: there is a '*', is that supposed to be some 
> kind of syntax indicator (e.g. repetition as in regular expressions)? 
> The example should be put into quotes.
> 
> 5.1.3, BATTERY_STATUS: "using a battery power" -> "using battery power"
> 
> 5.2: "and will treat the message if it was a normal ping" -> "and will 
> treat the message *AS* if it was a normal ping"
> 
> 5.4: I'm surprised that this diagnostics document has to define an error 
> code for "Underlay Destination Unreachable". Shouldn't such an error 
> condition occur in the base protocol already?
> 
> 5.5.1: There are way too many MUSTs here. It would be much better to put 
> limitations on field values where these field values are defined, and 
> just describe the limitations in positive language.
> 
> 5.5.1: The first paragraph uses "Ping message with diagnostic 
> extension", the second paragraph uses "diagnostic Ping message". Please 
> unify terminology.
> 
> 5.5.3: The first two paragraphs semantically form one single sentence. 
> The first paragraph forms one syntactical sentence. Both of these are 
> too long.
> 
> In the first paragraph, it is impossible to understand to what clauses 
> the initial "When" applies without rereading the paragraph. Please fix.
> 
> Section 6: Just to check, would this be what's described in this section:
> 
> <diagnostic-kind kind='0006'>  <!-- MACHINE_UPTIME -->
>    <access-node><!-- hex NodeID goes here --></access-node>
>    <access-node><!-- another hex NodeID may go here --></access-node>
>    <!-- more access-node elements as needed -->
> </diagnostic-kind>
> 
> Would <diagnostic-kind kind='6'> also be okay? Or should it be 
> <diagnostic-kind kind='0x0006'>? Things like these should be carefully 
> specified. An actual example also will help.
> 
> 8.1 and 8.5: Rather than creating two registries that have to be kept in 
> sync, it would be better to create one registry where the first 64 
> entries have more columns than the rest.
> 
> 8.6: "XML namespaces": I only see one namespace. Is the intent that the 
> example above actually be written e.g. like
> <diagnostic-kind kind='0006'
>      xmlns='urn:ietf:params:xml:ns:p2p:config-diagnostics'>
>    <access-node><!-- hex NodeID goes here --></access-node>
>    <access-node><!-- another hex NodeID may go here --></access-node>
>    <!-- more access-node elements as needed -->
> </diagnostic-kind>
> 
> Why is there a need for a separate namespace for just one or two 
> elements? It's a common misconception that each spec that defines a bit 
> of XML has to have its own namespace. As long as there is coordination 
> to avoid element name collisions (which should be achievable e.g. in an 
> IETF WG), there's absolutely no need for such piecemeal namespaces.
> 
> <mandatory-extension>: I'm not at all sure it's a good idea to use an 
> XML namespace URI to identify protocol/format extensions. But it may 
> already be too late to fix this.
> 
> 
> Regards,   Martin.
>