Hi Richard, Please see below.
> On 17. Feb 2020, at 05:44, Y. Richard Yang <[email protected]> wrote: > > Hi Mirja, > > Please see below. > > 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. > > > We are indeed designing an ALTO incremental based on HTTP/2 (/3). For now, we > plan to clarify that the design is for earlier HTTP and move 13.1 to Sec. 3. > How does this sound? It’s okay. However as you are already working on a design for HTTP/2 or HTTP/3, would it be better to finish this work up now and specify it from the beginning over this more modern protocols? Or is there a goos reason why HTTP/1 support is needed? > > 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! > > > Yes. All fixed. Thanks! > > > 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. > > > Excellent review. We have revised the paragraph to be the following: > > "JSON merge patch is intended to allow applications to update server > resources by sending only incremental changes. <xref target="RFC7396"/> > defines the encoding of incremental changes (called JSON merge patch objects) > to be used by the HTTP PATCH method <xref target="RFC5789"/>. This document > adopts from <xref target="RFC7396"/> only the JSON merge patch object > encoding and does not use the HTTP PATCH method.” Much better. Thanks! > > The goal of the revision above is to call out explicitly what we adopt---JSON > merge objects. What do you think? > > 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. > > > We are taking a pass across the document to clarify multipart, on both > motivation and full specification. A high-level guidance that we need is the > following: all existing (i.e., published) ALTO resources have a single JSON > object, but the extension, path vector (PV), is best handled by using > multipart, to avoid complex consistency handling: a resource consisting of > two related JSON objects. Hence, the SSE is revised to handle this. The > "non-standard ALTO service example" is based on PV but PV has not been > published and we do not want to create document dependency to slow down the > finish of SSE. How about the following two options: (1) we remove the > "non-standard example"; (2) we make clear that SSE can be used by both > current ALTO services and potential future services, and we label the > "non-standard" as just one example? I would propose to actually reference to the path vector draft. Just saing that is exists and using it in a (non-normative) examples, does not create a normative reference. > > 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. > > > Excellent suggestion. We will move it to the beginning of Sec. 5 with a > concrete global message flow specification, with the following new starting > paragraph of Sec. 5: > > " We now define the details of ALTO SSE. This section starts with the > specification > of the global message flow (<xref target="ALTO.SSE.MessageFlow"/>), > and then specifies the details of the data update messages (<xref > target="ALTO.SSE.UpdateEvents"/>) > and the control update messages (<xref > target="ALTO.SSE.ControlEvents"/>) respectively.” Thanks! Sounds good. > > 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. > > > Yes. To be precise, the server may use a subset of incremental encoding for a > given resource. To allow "old" client to still proceed, the client can set > the acceptance to be false. Here is a revision to try to clarify a bit more: > > If the "incremental-changes" field is "true", > the update stream server MAY send incremental changes for this > substream (assuming the update stream server supports > incremental changes for that resource). In this case, the > client MUST support all incremental methods from the set > announced in the server's capabilities for this resource; see Section > Section 6.4 > for server's announcement of potential incremental methods. If > a client does not support all incremental methods from the set > announced in the server's capabilities, the client can set > "incremental-changes" to "false", and the update stream server > then MUST NOT send incremental changes for that substream. Thanks for the clarification. That’s good. However, I was indirectly question if that is really the right design, rather than given the client also an option to indicate which of the methods it supports. Is there a reason why it’s design this was? Was that discussed in the group? > > 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? > > > Excellent catch. It should not be simply close the connection. We will > clarify all such error handling cases. Thanks! Yes, please double-check the rest of the document to be more clear about connection establishment and closure. > > 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)? > > > My understanding is that TCP keep alive may be off or can be quite long: I > just checked my Mac and got: > %sysctl -A | grep net.inet.tcp.*keep > net.inet.tcp.keepidle: 7200000 > net.inet.tcp.keepintvl: 75000 > net.inet.tcp.keepinit: 75000 > net.inet.tcp.keepcnt: 8 > net.inet.tcp.always_keepalive: 0 > > Let us dig a bit more and fix the final version. These parameters are also configurable. However, it really depends on the network you are in. If there is a NAT with short timers of not. Usually the default TCP values should covers to most common cases. > > > 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? > > > Very good point. We gave two approaches, and this sentence is for the second > approach: first update the > base (e.g., network map), mark the dependent (e.g., cost map) as invalid, and > wait for the dependent to be > updated. If the dependent update does not arrive, the system will be in an > invalid state. Since the base > has already been updated (committed, using a database term), there is no > rollback possible unless we also rollback > the base. This sentence gives only a simple transaction-based approach. Let > me clarify a bit more. So the the idea is that the establishment will send you the full map, right? However, it still seems like a client decision to do that or e.g. just give up; a MUST requirement seems a bit to strong. > > > 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. > > > OK. Fixed. Thanks. > > 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. > > OK. Fixed. Thanks. > > 10) section 10.2 > "Under overloading, the client may choose to remove the information > resources with high update rates.” > Should also be MAY. > > > OK. Fixed. Thanks. > > 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. > > > Yes. We are moving 13.1 to the front. Great! > > > 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? > > > > All the edits are fixed. Great! Thanks! Mirja > > Thank you so much! > > Thanks! > Mirja > _______________________________________________ alto mailing list [email protected] https://www.ietf.org/mailman/listinfo/alto
