OK, I'll file github issues On Fri, Aug 7, 2020 at 1:21 AM Michael Welzl <[email protected]> wrote:
> Hi Martin, > > Thanks much for this detailed review! - and for the many interesting > questions you’re raising; you discovered a few important holes there. E.g., > I believe we just haven’t discussed stream limits; I would have thought > that that’s an implementation matter, but the fact that you bring it up in > a review of the interface draft is a reminder that applications may want to > be aware of this behavior. > (as a quick answer to this particular item, in my opinion Clone() should > fail if the stream limit is exceeded - getting a mix of Connections and > streams, altogether grouped is surely not what anyone would want or expect; > e.g. priorities within the group won’t work out anymore). Note that the > idea is that an application can query for what it got if it’s interested in > that level of detail. > > Now, for addressing the comments, I think we should just split them up by > topic into issues for the repository, and then address them one by one. > > Thanks again! > > Cheers, > Michael > > > On Aug 6, 2020, at 9:30 PM, Martin Duke <[email protected]> wrote: > > Here is my promised "no prior knowledge" review of taps-interface. I > haven't filed issues for these yet, as I suspect some of these have already > been well-litigated there. > > The only other draft I've read is taps-arch. > > *High level comments*: > > (1) I think this conceptually headed in the right direction. > > (2) I found the relationship between streams and connections to be > extremely murky throughout the draft, and left it not really sure how I > would control a streamed transport. > > Section 6.4 hints suggests that opening additional streams is an > alternative implementation of Clone() for transports that can do so. Does > this imply that if the peer stream limit does not allow new streams, the > transport should open a connection instead? How does > SetNewConnectionLimit() in Sec 6.2 relate to streamed transport -- are > there two levels of listener: a stream listener and a connection listener? > How would opening "stream listeners" relate to the maximum number of > streams the peer could open (i.e. do I have to call listen() 8 times to > allow the client to open 8 streams)? Or are there no stream listeners and > these "connections" just sort of appear? > > Skimming the -impl draft, I can see there's some text here but it is not > fitting together for me. There are at least three streamed transports and > it would be helpful to explain how the API conceptualizes this, perhaps in > a new section. > > The next two major points are related to streams, so if I've missed > something important above they may be complete nonsequiturs. > > (3) I'm not sure the priority framework really fits together. > > Sec 6.4 says we 'should' do something that sounds like WFQ, but 7.1.5 > allows the application to decide what happens. Meanwhile message priority > suggests a strict priority-based scheduler. > > Meanwhile, in sec 8.1.3.2 "Note that this property is not a per-message > override of the connection Priority - see Section 7.1.3. Both Priority > properties > may interact, but can be used independently and be realized by > different mechanisms." > > At the risk of overindexing on a partly-baked draft in httpbis, how would > I implement strict priority of HTTP/3 streams over a taps API that > implements QUIC? Would I need to request strict priority in *both* > connection priority and message priority? Or I guess connection priority > would be sufficient? > > (4) I found myself continually asking how I would implement > foo-over-http/x using this API, and very much stumbled over ambiguity > regarding HTTP 1.1 pipelining. While it's certainly possible to order > messages in the proper way using this API, if I was writing this > agnostically to HTTP version, it appears it would gravitate to opening a > pooled TCP connection for each request/response if we ended up on HTTP/1 > [because each request/response would have a clone() call]. Maybe this is > OK, but I found this conclusion to be jarring. > > *Specific comments*: > > sec 4.2.2 > "implementations MUST provide a > method for the application to determine the entire set of possible > values for each property." > > Is this meant to be a requirement for a machine-readable API for learning > the enum, or merely a requirement for documentation? i.e. is it > satisfied if the values are in the man page or comments of the C header > file? > > sec 5. > I found it curious that there is no API here for registering callbacks, or > mention that this is a precondition to connection. Surely event handlers > are a prerequisite? > > sec 5.1 > I found it odd that there didn't appear to be any sort of formal list of > attributes of an endpoint, just some examples. > > sec 5.2.7 > I am not sure what this sentence means: "Disabling this property may > enable to control checksum coverage later." > > sec 5.2.9 > It would be helpful to say "Applications that neither prohibit nor require > congestion should query the result and disable their own congestion control > if present in the transport" > > sec 5.2.12 > Using temporary local addresses may break various mechanisms that prove > address ownership (e.g QUIC resumption tokens) and therefore impair > performance as the client has to re-verify its address. > > sec 6.2 > "After Stop() is called, the Listener can be disposed of.." I don't > understand the difference between calling listener.stop() and simply > disposing of the object. Does this imply that there is a listener.start() > that allows a configured listener to resume? > > sec 7.1.4 > It would be helpful to clarify the relevance of control vs application > data in the idle timeout -- for various reasons, the connection could stay > alive regardless of how active the connection is (in fact, perhaps > "keepalive" is a generally useful notion that can be exposed in the API?) > > sec 7.1..8 > This section is missing a reference to minSendRate and minRecvRate > > sec 7.1.9. > We're going to need an event for MTU changes. These can't simply be Send > errors because the Sent event might go first, or the transport might be > sending a probe with padding that doesn't correspond to a message. > > sec 7.2 > perhaps I'm not thinking of this correctly, but should there be a way to > query the peer-offered user timeout? > > sec 8.3.2.1 > Reading the Sent event, I was pretty sure that reliable transports would > deliver the event until the ack arrived. But Sec 10 most definitely does > not say that. I would think that the ack would be a more useful signal, but > maybe I'm in the rough on this -- certainly many transport implementations > will not do this well. > > -Martin > _______________________________________________ > Taps mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/taps > > >
_______________________________________________ Taps mailing list [email protected] https://www.ietf.org/mailman/listinfo/taps
