Hi Richard, Please see inline.
> On 17. Feb 2020, at 21:03, Y. Richard Yang <y...@cs.yale.edu> wrote: > > Hi Mirja, > > Thanks a lot for the ultra-fast response. Please see below. > > On Mon, Feb 17, 2020 at 4:21 AM Mirja Kuehlewind <i...@kuehlewind.net> wrote: > Hi Richard, > > Please see below. > > > On 17. Feb 2020, at 05:44, Y. Richard Yang <y...@cs.yale.edu> wrote: > > > > Hi Mirja, > > > > Please see below. > > > > On Fri, Feb 14, 2020 at 4:32 PM Mirja Kuehlewind <i...@kuehlewind.net> > > 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? > > Thanks. We will finish this work now, for HTTP/1/1.1, as it is mostly done > and HTTP/1/1.1 is widely adopted. . HTTP/2 is definitely gaining momentum > (https://w3techs.com/technologies/details/ce-http2 states 43.3% in Feb. 2020) > and we will do the work during the next phase as soon as we are done with > this one. I don’t think that is a relevant number for alto because that's about Webservers already offering http/2. And I would assume an alto server is usually not co-located with another Webserver. For alto the most important question would be if the http sever implementation you are using is supporting http/2 and I think in terms of implementation most platforms do. So if you run an alto server and want to use SSE with http/2 you need to be willing to update you server; that’s it. Anyway, I think you need a bit more justification in the draft why this is http/1.1 based. > > > > > > 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. > > > OK. Sounds good and will do now. > > > > > 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? > > > It was discussed in the WG. > > A "standard negotiation" protocol is: > A -> B: I support set SA > B -> A: I pick subset SB \subset SA > > The decision, after the discussion, was that the following design: > S -> C (IRD): this SSE supports set S + full-replacement > C -> S (in request): I accept S+full-replacement (true) or only > full-replacement (false) > > The discussion was that the server (1) has more information (for example, > predict/know the set of potential changes, and hence predict either using > merge patch or json patch is more efficient), and (2) is more likely to be > the bottleneck (e.g., pushing updates to multiple clients). Hence the > decision is to allow the server to make the choice to (1) more likely choose > a more efficient encoding, and (2) sharing of the encoding overhead (if > client C1 chooses encoding e1, and C2 chooses e2, then the server needs to > compute both e1 and e2). Switching the design to the "standard negotiation" > is a simple modification, and I am actually relatively open to this, but I do > feel that the current design is simpler. The authors and others can sure > discuss the issues during the weekly meeting. Any remaining comments? Okay, maybe add some more explanation in the draft. > > > > > > 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. > > > WIll do. > > > > > 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. > > > Yes. After close, which is indeed a client decision, and re-establishment, > the server will send the full dependent of the dependent to allow recovery. I > agree that MUST is a bit strong. A client with a higher level of patience can > indeed wait. Let us change to "MAY choose”. Or SHOULD is fine as well. Maybe rather add a bit more explanation when it might be appropriate to not re-connect > I will also add that such timeout has the traditional transport timeout > caveat, too quick timeout can lead to attack to the server. How does this > sound? That’s actually a really good point, about the timeout. Please add. I guess the timeout should actually be a SHOULD or even MUST. > > We will send in a new version with all the changes by mid this week (Thursday > the lastest), if you see the approaches as we discussed are OK. That would be great. I think you can just go ahead and submit the new version to the datatracker, so I can start IETF last call right away (if everything is okay). Thanks! Mirja > > Thank you so much!! > > Richard > > > > > 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 > > > > > > -- > -- > ===================================== > | Y. Richard Yang <y...@cs.yale.edu> | > | Professor of Computer Science | > | http://www.cs.yale.edu/~yry/ | > ===================================== _______________________________________________ alto mailing list alto@ietf.org https://www.ietf.org/mailman/listinfo/alto
