Dear Elwyn, Thanks a lot for the review! Please see inline below.
On Mon, Mar 9, 2020 at 8:45 PM Elwyn Davies via Datatracker < nore...@ietf.org> wrote: > Reviewer: Elwyn Davies > Review result: Almost Ready > > I am the assigned Gen-ART reviewer for this draft. The General Area > Review Team (Gen-ART) reviews all IETF documents being processed > by the IESG for the IETF Chair. Please treat these comments just > like any other last call comments. > > For more information, please see the FAQ at > > <https://trac.ietf.org/trac/gen/wiki/GenArtfaq>. > > Document: draft-ietf-alto-incr-update-sse-20 > Reviewer: Elwyn Davies > Review Date: 2020-03-09 > IETF LC End Date: 2020-03-06 > IESG Telechat date: 2020-03-12 > > Summary: > Almost ready. There are a few editorial issues, but I am not sure that > > Major issues: > I am unsure whether this mechanism is proof against loss of messages or > reordering of messags. Although there are state tags, it does not appear > to > have any way to ensure that the state to which the updates will be applied > in > the client are identical to the state that the updates were generated > from. If > I am wrong, it would be useful (IMO) to explain how the proposal avoids > getting > updates that don't apply to the state in the client. > Good comment! A short answer is that the design should have no consistency problems. More details: (1) This design is based on http/1.x as transport, which provides a single, reliable, in-order serialization of update messages: m1, m2, m3, ... The transport will guarantee that the messages will be delivered lossless, in order. (2) One can consider that the messages consist of substreams (resources). Each substream is total ordered as well. (3) The only remaining case is that substreams can have dependencies: for example a cost map can depend on a network map. The design requires that the updates to such dependencies are ordered correctly. One can see that the consistency model can be weakened: from total serialization to causal consistency. We plan to design such a weaker (with less head of line blocking of total order) using http/2. I like this comment. How about that we add a realized consistency model paragraph in the overview? What do you think? > Minor issues: > > Nits/editorial comments: > Abstract: the abstract is too long; I would suggest deleting the second > sentence of the first paragraph and the whole of the second paragraph. Ths > would leave sufficient information to explain what the document proposes > but > omits the rationale which is not necessary for outlining the contents. > The > deleted text would be usefully incorporated into s1. Okay. > > Abstract, para 3: s/s ction/section/ Thanks. Will fix. > > s1: The key role of Server-Sent Events in this proposal is not introduced > here > (and isn't mentioned in the Abstract). In the process SSE needs to be > expanded > on first use (currently right at the end of the section) and a pointer to > the > document that defines SSE [SSE] Okay. > > s1, last para: The reference to Section 13 should come right at the end - > and > the last two sections are (no longer) the last sectons: s/last two > sections/Sections 11 and 12/ Thanks a lot for identifying this. Will fix. > > s2 et seq: I am unsure of the rationale for defining a set of special > terms and > not capitalizing them on every occurrence. We feel that this is a style preference. We intended that the terms in Sec 2 are like keywords of a book. Capitalizing them on each occurrence appears to be a bit too much, for personal style. We prefer to keep this style, but do agree that some other ALTO documents use all capitalization. > > s2: There is quite a lot of terminology imported from RFC 7285 . This > should > be mentioned. > Good catch. Will add a sentence at the beginning. > s3: A pointer to the SSE document would be useful [SSE]. > Yes. Will do. > s3.4: It would be better to use the expanded form of SSE in the first > paragraph > rather than waiting till the 2nd para. Sure. Will do. > > s4: An explanation in advance of the format of the lines delineated by > **.... > ** would be desirable. > Sure. > s5.1, next to last para: s/ So there is no ambiguous decoding/ So there > is no > ambiguity when decoding/ > Good revision and will do. > s5.1, last para: s/id/data-id/ Good catch, and will fix. > > s6.3, last para: s/will uses/will use/ Thanks. Will fix. > > s6,5, "incremental changes": s/Section Section6.3/Section 6.3/ Thanks. Will fix. > > s6.5, "remove": Stating that the client SHOULD ignore this if it present > is > potentially problematic. If it is there it is a syntax error - should the > message be ignored and potentailly flagged as an error? The overall design strategy of alto is to ignore unknown fields to allow incremental deployment—a kind of future proof of a future version by a legacy old version. But in this case, I agree that it is a known error and it is a good clarification. We will flag it as an error. > > s7.6, last para: s/our modular/the modular/ Thanks. Will fix. > > s13/s13.1:Empty sections are not desirable Please combine the two titles > and > remove s13.1 . > Okay. Thanks again! Richard
_______________________________________________ alto mailing list alto@ietf.org https://www.ietf.org/mailman/listinfo/alto
