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 <[email protected]> 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 <[email protected]> | | Professor of Computer Science | | http://www.cs.yale.edu/~yry/ | =====================================
_______________________________________________ alto mailing list [email protected] https://www.ietf.org/mailman/listinfo/alto
