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

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

Hi Mirja,

We have submitted a new version. If you need to move on forward this week,
please use the submitted version. If you can do so by Monday morning, then
we plan to take a couple reads during this weekend.

Your reviews helped greatly and we greatly appreciate them!

Richard

On Tue, Feb 18, 2020 at 6:46 PM Y. Richard Yang <[e-mail]> wrote:

> Hi Mirja,
>
> I believe that we have a grasp of all of your feedback. We will submit in
> the next couple days, a new version directly to the data tracker, as you
> suggested.
>
> Thank you so much! Really appreciate it!
>
> Richard
>
> On Tue, Feb 18, 2020 at 4:32 AM Mirja Kuehlewind <[e-mail]>
> wrote:
>
>> Hi Richard,
>>
>> Please see inline.
>>
>> > On 17. Feb 2020, at 21:03, Y. Richard Yang <[e-mail]> 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 <[e-mail]>
>> wrote:
>> > Hi Richard,
>> >
>> > Please see below.
>> >
>> > > On 17. Feb 2020, at 05:44, Y. Richard Yang <[e-mail]> wrote:
>> > >
>> > > Hi Mirja,
>> > >
>> > > Please see below.
>> > >
>> > > 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.
>> > >
>> > >
>> > > 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
>>
>