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
_______________________________________________
alto mailing list
alto@ietf.org
https://www.ietf.org/mailman/listinfo/alto
