Subject: [ppsp] Barry Leiba's Discuss on draft-ietf-ppsp-base-tracker-protocol-10: (with DISCUSS and COMMENT)

From: "Barry Leiba" <[email-masked]>
Date: Wed, 14 Oct 2015 21:31:05 -0700

Barry Leiba has entered the following ballot position for
draft-ietf-ppsp-base-tracker-protocol-10: Discuss

When responding, please keep the subject line intact and reply to all
email addresses included in the To and CC lines. (Feel free to cut this
introductory paragraph, however.)

Please refer to https://www.ietf.org/iesg/statement/discuss-criteria.html
for more information about IESG DISCUSS and COMMENT positions.

The document, along with other ballot positions, can be found here:
https://datatracker.ietf.org/doc/draft-ietf-ppsp-base-tracker-protocol/

----------------------------------------------------------------------
DISCUSS:
----------------------------------------------------------------------

You use RFC 2616 as a reference for HTTP 1.1, but that's obsolete, and
the current reference should be RFC 7230 for the general references and
RFC 7231, Section 3.1.1.5 for Content-Type (the citation in Section 4
here).  The Security Considerations section should cite both 7230 and
7231.

-- Section 3.1 --

   A JSON object consists of name/value pairs.  The JSON names of the
   pairs are indicated with "".

I don't find the meaning of the second sentence to understandable at all.
 What are you trying to say?

-- Subsections of 3.2 --
Where are the semantics of the string values, such as "HIGH", "NORMAL",
and "LOW", defined?

-- Section 4 --

   For deployment scenarios where Peer (Client) authentication is
   desired at the Tracker, HTTP Digest Authentication MUST be supported,
   with TLS Client Authentication as the preferred mechanism, if
   available.

You need a normative reference to RFC 7616 for HTTP Digest
Authentication, and a citation here.

-- Section 4.1.1.1 --
Please re-check your examples carefully, and pass them through a JSON
validator.  In the first example, for instance, the Content-Length value
is wrong and the "transaction_id" member is missing its ":".  The third
example uses "TransactionID" instead of "transaction_id" for the member
name.

Be careful during AUTH48 that you re-check this, including making sure
that any Content-Length values are still correct after all the editing.

----------------------------------------------------------------------
COMMENT:
----------------------------------------------------------------------

Is the definition give for "LEECH" sufficient?:

   LEECH:  A Peer that has not yet completed the transfer of all Chunks
   of the media content.

I don't think "completed the transfer" is clear enough, and "transfer"
can be outbound or inbound.  Would something like "A Peer that has not
yet received all chunks of the media content, and therefore can't be used
to share the content," be better?  And isn't it possible for a peer to
share the chunks that it has, even if it doesn't have all of the chunks?

You do seem inconsistent with capitalization of terms ("SEEDER" and
"Seeder", "LEECH" and "Leech", "SWARM and "Swarm" and "swarm").  You
should fix that -- readers often find it confusing, and the RFC Editor
will ask you about it during AUTH48.

-- Section 1.2.1 --
I don't understand the meaning of "[Peer Protocol]" in front of list
items 3, 4, and 5.  Why's it there?  And if you're going to leave it
there, I think you might do better to find notation that doesn't look
like how we write reference citations.

-- Section 2.2 --

   Requests are sent, and responses returned to these requests.  A
   single request generates a single response (neglecting fragmentation
   of messages in transport).

The word "neglecting" can make this ambiguous -- it can be taken to mean
that fragmentation can't happen.  It might be better to say it this way:

NEW
   Requests are sent, and responses returned to these requests.  A
   single request generates a single response (though both requests
   and responses can be subject to fragmentation of messages in
   transport).
END

-- Section 4.3 --

   If the peer fails to read the tracker response, the same Request with
   identical content, including the same transaction_id, SHOULD be
   repeated, if the condition is transient.

...and...

   The tracker SHOULD be prepared to receive a Request with a repeated
   transaction_id.

Given the first paragraph, why isn't the SHOULD in the second one a
MUST?

In the list of error responses, why are some of them MUST and others
SHOULD?

-- Section 4.4 --
There are no JSON things called "fields".  Objects have "members" and
arrays have "elements".  I think you mean to use "members" in this
section.

-- Section 7 --

   Additionally, a designer MUST remember that extensions themselves MAY
   also be extensible.

The "MAY" shouldn't be a 2119 key word, and should be changed to lower
case.

-- Section 8.1 --
This is not defining a registry; it's making a registration in the media
types registry.  We no longer use the term "MIME type", so please change
the title to "Media type registration", and change the first paragraph:

OLD
   This document defines registry for application/ppsp-tracker+json
   media types.
NEW
   This document registers the application/ppsp-tracker+json
   media type.
END

You also use "MIME type" in the "File extension" information (you can
actually just change that to "n/a").  And you haven't updated the
registration template to the latest one specified in RFC 6838 -- there's
a new "Fragment identifier considerations" item (which can just be "n/a"
also, but please add it).

-- Section 10 --
Some of your informative references are for required functions, and
should be normative.  In particular, RFC 2616 needs to be changed to 7230
and 7231 (see above) and needs to be normative.  2818 and 5246 should be
normative.  3986 and 5952 tell you how to encode IP addresses, and should
be normative.

---

From: "Huangyihong (Rachel)" <[email-masked]>
Date: Mon, 19 Oct 2015 09:01:25 +0000

Hi Barry,

Thank you for your valuable comments. They all make sense to us. Please see my replies inline.

BR,
Rachel

> -----Original Message-----
> From: Barry Leiba [mailto:[email-masked]]
> Sent: Thursday, October 15, 2015 12:31 PM
> To: The IESG
> Cc: [email-masked]; [email-masked];
> Zongning; [email-masked]
> Subject: Barry Leiba's Discuss on draft-ietf-ppsp-base-tracker-protocol-10:
> (with DISCUSS and COMMENT)
> 
> When responding, please keep the subject line intact and reply to all email
> addresses included in the To and CC lines. (Feel free to cut this introductory
> paragraph, however.)
> 
> 
> You use RFC 2616 as a reference for HTTP 1.1, but that's obsolete, and the
> current reference should be RFC 7230 for the general references and RFC 7231,
> Section 3.1.1.5 for Content-Type (the citation in Section 4 here).  The Security
> Considerations section should cite both 7230 and 7231.

[Rachel]: Right. Will fix it.

>  What are you trying to say?

[Rachel]: Actually we mean name = quotation-mark *char quotation-mark. [RFC7159] has specified it, so how about we delete the related sentences?

> 
> -- Subsections of 3.2 --
> Where are the semantics of the string values, such as "HIGH", "NORMAL", and
> "LOW", defined?

[Rachel]: Right. And I think it's hard to define any empirical value in this document. So how about changing the meaning of the 4 optional attributes to the requesting peer's status? So that concurrent_links, online_time, upload_bandwidth can be changed to ppsp_tp_integer_t type. Thus, tracker can do some peer selection based on peer's current status? 

New
"
Object {
              ppsp_tp_integer_t   peer_count;  // the
              [ppsp_tp_string_t   ability_nat = "NO_NAT"
                                              | "STUN"
                                              | "TURN";]
              [ppsp_tp_integer_t   concurrent_links;]    // the peer's links number
                                                  
              [ppsp_tp_integer_t   online_time;]  // the peer's online time
              [ppsp_tp_integer_t   upload_bandwidth;]  // the peer's upload bandwidth
      } ppsp_tp_peer_num_t;

"

> You need a normative reference to RFC 7616 for HTTP Digest Authentication,
> and a citation here.

[Rachel]: Will do.
> validator.  In the first example, for instance, the Content-Length value is
> wrong and the "transaction_id" member is missing its ":".  The third example
> uses "TransactionID" instead of "transaction_id" for the member name.

[Rachel]: Will do.
> 
> Be careful during AUTH48 that you re-check this, including making sure that
> any Content-Length values are still correct after all the editing.
> 
> 
> can be outbound or inbound.  Would something like "A Peer that has not yet
> received all chunks of the media content, and therefore can't be used to share
> the content," be better?  And isn't it possible for a peer to share the chunks
> that it has, even if it doesn't have all of the chunks?

[Rachel]: A peer can share the received chunks even if it doesn't have all the chunks. So how about changing to "A peer that has nothing or has not yet received all chunks of the media content"?
> 
> You do seem inconsistent with capitalization of terms ("SEEDER" and "Seeder",
> "LEECH" and "Leech", "SWARM and "Swarm" and "swarm").  You should fix
> that -- readers often find it confusing, and the RFC Editor will ask you about it
> during AUTH48.

[Rachel]: Ok. Will do it in the next version.

> 
> -- Section 1.2.1 --
> I don't understand the meaning of "[Peer Protocol]" in front of list items 3, 4,
> and 5.  Why's it there?  And if you're going to leave it there, I think you might
> do better to find notation that doesn't look like how we write reference
> citations.
> 

[Rachel]: Sorry, it should be changed to [RFC7574] which should be added into the reference section.

> The word "neglecting" can make this ambiguous -- it can be taken to mean that
> fragmentation can't happen.  It might be better to say it this way:
> 
> NEW

[Rachel]: Good suggestion. Thanks.

> Given the first paragraph, why isn't the SHOULD in the second one a MUST?
> 

[Rachel]: Right. They all should be changed to "MUST". 

> 
> -- Section 4.4 --
> There are no JSON things called "fields".  Objects have "members" and arrays
> have "elements".  I think you mean to use "members" in this section.

[Rachel]: Yes. It should be "members".

> The "MAY" shouldn't be a 2119 key word, and should be changed to lower case.

[Rachel]: Right.

> 
> -- Section 8.1 --
> This is not defining a registry; it's making a registration in the media types
> registry.  We no longer use the term "MIME type", so please change the title
> to "Media type registration", and change the first paragraph:
> 
> OLD

[Rachel]: Your suggestion makes sense to me.
> 
> You also use "MIME type" in the "File extension" information (you can actually
> just change that to "n/a").  And you haven't updated the registration
> template to the latest one specified in RFC 6838 -- there's a new "Fragment
> identifier considerations" item (which can just be "n/a"
> Some of your informative references are for required functions, and should be
> normative.  In particular, RFC 2616 needs to be changed to 7230 and 7231
> (see above) and needs to be normative.  2818 and 5246 should be normative.
> 3986 and 5952 tell you how to encode IP addresses, and should be normative.
> 
[Rachel]: Ok. Will fix it in the next version.

---

From: Barry Leiba <[email-masked]>
Date: Mon, 19 Oct 2015 11:13:37 -0400

Hi, Rachel, and thanks for addressing my comments.

> [Rachel]: Actually we mean name = quotation-mark *char quotation-mark.
> [RFC7159] has specified it, so how about we delete the related sentences?

Yes, that seems best; I'll have another look after you're updated the document.

> [Rachel]: Right. And I think it's hard to define any empirical value
> in this document. So how about changing the meaning of the 4 optional
> attributes to the requesting peer's status? So that concurrent_links,
> online_time, upload_bandwidth can be changed to ppsp_tp_integer_t
> type. Thus, tracker can do some peer selection based on peer's current
> status?

Your suggestion here seems fine, if you add a few words to give a
sense of what the integer value might mean (realizing that it's going
to be vague, and that vague is OK).

> [Rachel]: A peer can share the received chunks even if it doesn't have
> all the chunks. So how about changing to "A peer that has nothing or
> has not yet received all chunks of the media content"?

That's fine; thanks.

Barry