Re: [alto] AD review of draft-ietf-alto-incr-update-sse-19

["Y. Richard Yang" <[e-mail]>]

Hi Mirja,

Thanks a lot for the wonderful review! This is an ack that the authors have
received the reviews and will send a full plan on how to address each item
of the reviews by this weekend.

Cheers,
Richard

On Fri, Feb 14, 2020 at 4:32 PM Mirja Kuehlewind <[e-mail]>
wrote:

> Hi authors,
>
> I reviewed draft-ietf-alto-incr-update-sse in order to potentially get
> this document through the process before I step down as AD in March. I have
> a couple of technical questions below that I would some feedback about
> before we move on to IETF last call and some fully editorial point that can
> be updated anytime before final publication (e.g. also after IETF last call
> has ended).
>
> However, I have one high level question that I would like to at least
> understand to justify it in the IESG before we moved on. Sorry that I have
> not raised this earlier in the working group but I realised it just now:
>
> HTTP/2 (and HTTP/3) support multiplexing, why is a separate service needed
> for stream control, instead of just using a different stream on the same
> HTTP connection?
>
> Also note that I don’t find the justification, why HTTP/2 Server Push is
> not used, not very convincing and I’m afraid to get push-back from the ART
> ADs. Especially given the problems describes in section 9.5. However, we
> can give it a try. I think I would recommend to have the justification in
> section 13.1 earlier in the document, e.g. in section 3.
>
> And one more request before we move on, please add the actual
> Content-Length values to the examples and confirm that you did run the
> examples though a syntax checker!
>
>
> Here are my other technical questions/comments:
>
> 1) In section 3.2.1
> "This document adopts
>    the JSON merge patch message format to encode incremental changes,
>    but uses a different HTTP method, i.e., it uses POST instead of
>    PATCH.”
> I don’t quite understand this sentence. As you use SSE, I guess you are
> using neither POST nor PATCH. The point here is that SSE has a quite
> different communication model than otherwise in HTTP, so I’m not sure what
> this statement actually tells me.
>
> 2) Why is the multipart content-type supported if in [RFC7285], each
> "resource is encoded as a single JSON object" (as you state in section 5.2)?
> In relation to this question, I have to say that I find it also rather
> inappropriate for an RFC that section 8.1 shows a "non-standard ALTO
> service” in the example.
> Further I was surprised to see this line in section 8.5:
> "event: multipart/related;boundary=...”
> This doesn’t seem to be sufficiently specified in the draft…? Can you
> please further explain in the draft.
>
> 3) section 5.3:
> "The server MUST send a control update message
>         with an URI as the first event in an update stream. “
> I missed this statement on first read. I think it’s not great to have this
> as port of the message format definition. I would proposed to move this to
> the beginning of section 5 and also add some more text on the general
> message flow. I know this is described (non-normatively) in section 4,
> however, section 4 does rather describe the function and services provided
> and not really the concrete message flow.
>
> 4) In section 6.3 you say:
>        "An
>         alternative design of incremental-changes control is a more
>         fine-grained control, by allowing a client to select the subset
>         of incremental methods from the set announced in the server's
>         capabilities (see Section Section 6.4).”
> Does that mean that a client that wants to use incremental changes must
> support all methods implemented by the server? That seems to make
> deployment of new methods rather complicated.
>
> 5) section 6.6:
> "The update stream server MUST close the stream without
>       sending any events.”
> What does "close the stream" actually mean? Close the HTTP/TCP connection?
>
> 6) One question related to section 6.8: Please note that TCP also provides
> a Keep-alive function. Is there any explanation in [SSE] why that is not
> used or recommended (sorry didn’t had time t look this up in [SSE] myself
> and could find anything quick)?
>
> 7) section 9.2:
>   "However, if the ALTO client does not receive
>    the expected updates, the ALTO client MUST close the update stream
>    connection, discard the dependent resources, and reestablish the
>    update stream.”
> Why is this a MUST requirement?
>
> 8) section 10.1:
>   "To avoid these attacks on the update stream server, the server MAY
>    choose to limit the number of active streams and reject new requests
>    when that threshold is reached.”
> This however should be a SHOULD rather than a MAY.
>
> 9) Also section 10.1:
>   "Therefore an update stream server may prefer to restrict update
>    stream services to authorized clients, as discussed in Section 15 of
>    [RFC7285].”
> This should probably be also a normative MAY.
>
> 10) section 10.2
>   "Under overloading, the client may choose to remove the information
>    resources with high update rates.”
> Should also be MAY.
>
> 11) Section 13.2 states that a stream can not be re-started. However this
> is not really mentioned explicitly in the body of the document. I think the
> text in 13.2 needs to be moved to somewhere earlier in the document and in
> addition clearly state what is the expected behaviour when a connection
> fails.
>
>
> And the editorial comments:
>
> 0) Abstract: For an RFC the abstract is rather long. I’d suggest to simply
> remove the second paragraph (as it is repeated anyway in the intro).
>
> 1) Please spell out IRD at first occurrence.
>
> 2) This document uses a couple of times “we”. While it is not forbidden to
> do so, it rather uncommon for an RFC (as “we” is not fully clear and should
> rather refer to the wg as a whole than the authors only). In many cases
> “we” can be simply replaced by “in this section”. In other case “we” can
> basically simply be removed, like here in the intro:
>
> OLD
> "We intend that the mechanism can also
>    support new ALTO services to be defined by future extensions…”
>
> NEW
> “The mechanism can also
>    support new ALTO services to be defined by future extensions…”
>
> 3) section 5.2:
> “...encoded using multipart/related [RFC2387].”
> Is the word "Content-type” missing here? This occurs several times in the
> document but not sure you say it that way...
>
> 4) also section 5.2:
> "Each component requiring the service of this document
>    MUST be identified by a unique Content-ID to be defined in its
>    defining document.”
> I’m not sure I understand this sentence? What do you mean by “service of
> this document” and “to be defined in its defining document”?
>
> 5) section 5.2 nits:
> "The encoding of a full replacement
>    is defined its defining document (e.g., network and cost map messages
>    by [RFC7285], and uses media type defined in that document.”
> Missing ending brackets and s/media type/the media type/ or “media types
> defined in those documents”
>
> 6) section 6.3:
> "the resource-id of an ALTO resource, and MUST be in the
>         update stream's "uses" list (Section 8.5.2 of Section 6.5)”
> There is a broken reference here.
>
> 7) section 6.3:
> "The default value for "incremental-
>         changes" is "true", so to suppress incremental changes, the ALTO
>         client MUST explicitly set "incremental-changes" to "false”.”
> This should be two sentences -> full-stop after “true”
>
> 8) section 6.7.1:
> “…to stop updates for one or more resources Section 7,…”
> Missing bracket -> (Section 7)
>
> 9) section 7:
> "If the
>    ALTO client and the stream control server the ALTO client MAY send
>    multiple stream control requests to the stream control server using
>    the same HTTP connection.”
> There is something missing in this sentence…?
>
> 10) Sec 8.3:
> "POST /updates/streams/2718281828459" HTTP/1.1”
> -> remove " after path
>
> 11) It seems like section 11 should be earlier in this document, maybe
> somewhere as subsection in section6?
>
>
> Thanks!
> Mirja
>
>
>
>
>
>

-- 
-- 
 =====================================
| Y. Richard Yang <[e-mail]>   |
| Professor of Computer Science       |
| http://www.cs.yale.edu/~yry/        |
 =====================================