Hello Martin, First of all thank you for your thorough review and comments. A new version has been uploaded to address them and is available at https://datatracker.ietf.org/doc/html/draft-ietf-alto-multi-cost-09 I hope the updates address your questions. Please, let me know otherwise. Please see also inline for my answers, Best regards, Sabine >>-----Original Message----- >>From: Martin Thomson [mailto:martin.thomson@xxxxxxxxx] >>Sent: 06 April 2017 04:08 >>To: art@xxxxxxxx >>Cc: alto@xxxxxxxx; ietf@xxxxxxxx; draft-ietf-alto-multi-cost.all@xxxxxxxx >>Subject: Artart telechat review of draft-ietf-alto-multi-cost-08 >> >>Reviewer: Martin Thomson >>Review result: Ready with Issues >> >>Document: draft-ietf-alto-multi-cost-08 >>Date: 2017-04-06 >>Reviewer: Martin Thomson >> >>This document describes how ALTO can be used to acquire cost maps with >>multiple cost metric instead of a single metric. >> >>The document very carefully deals with backwards compatibility with existing >>ALTO servers and clients. I don't anticipate many issues arising from the >>deployment of this protocol. >> >>I started reading -07, but finished with -08. I checked that the issues I raise >>still exist, but I'm not infallible. Apologies if I get something wrong. >> >>Minor issues: I've identified a few issues that are more than nits, these are >>marked "IMPORTANT" below. >> >> >>The abstract includes considerable justificiation. An abstract only needs to >>describe the *what*, not the *why*. Thus, what is there could be simplified >>considerably, e.g., >> >> This document defines a new method for retrieving multiple cost metrics in >>a >> single request for an ALTO filtered cost map. It also defines improvements >> to the constraints that can be used for filtered cost maps. [SR ] OK. Abstract shortened in that direction >> >>Section 1 >> >>The introduction uses a bunch of odd terms. Some of these are recognizable >>from the ALTO specification, but some of the jargon seems unnecessary. In >>particular, "Internet View", "Provider Network region" and "Vector costs". >>All of which I think that I understand, but they make the doc hard to follow. >> >>Generally, I found the introduction quite hard to follow, both for that reason >>and structurally. The introduction could be a lot shorter and more >>concise: >> >>1. ALTO defines multiple cost types (and more are being defined). >>2. Clients sometimes consume multiple cost types. >>3. Requesting multiple cost types at the same time is more efficient (for >> several reasons). >>4. This document defines how to do that. >>5. Separately, when multiple cost types are present, more sophisticated >> filtering can improve efficiency further. >>6. This document defines how to do that too. >> [SR ] OK, section revised in that direction >>Section 2 >> >>There are several items in the list here that are not used: >>Application Client, >>Network Service Provider, maybe more. Please check and remove those >>that don't apply. >> [SR ] OK: removed 3 definitions >>The RFC 7285 section reference thing is unnecessary. >> [SR ] OK: moved to end of section 2, to avoid repeating "of [RFC7285]" too often. >>This document doesn't cite RFC 2119, but it uses the keywords. [SR ] RFC 2119 is cited on page 1, section " Requirements Language" and section "9.1. Normative References". Should it be referenced elsewhere? >>Section 3.1 >> >>The example shows an empty cost-type, but the schema you define allows it >>to be absent. You REALLY need to pick one. I don't believe that this is a >>compatibility issue: once you have determined that a client supports multi- >>cost, then you can do anything you like, just be clear about it. [SR ] OK, added explanations before and after the example >> >>Section 3.2 >> >>I found the argument about the ease of writing a parser to be quite >>unconvincing. However, a new media type that is largely the same as the >>existing media type won't necessarily result in code duplication. >> >>Just say what it is you expect to happen and don't try to be apologetic about >>it. What you have appears to be a workable design. [SR ] OK, removed 1st paragraph. >> >>Section 3.5 >> >>This section is confusing. You only need to say that you are not altering full >>cost map resources in any way and that clients need to use filtered cost maps >>if they want multiple costs at the same time. (Obviously you could have, but >>creating multiple resources with the full combinatorial mess caused by >>combining many cost types is unwieldy.) At a minimum, the second >>paragraph here can be removed. [SR ] OK shortened 2nd paragraph >> >>Section 3.6.2 >> >>IMPORTANT: You don't define what happens when a client provides "or- >>constraints" >>and "constraints" at the same time. There are several valid options, but you >>need to choose. [SR ] OK added text at the end on why a client cannot provide both >> >>Section 3.6.3 >> >>It is probably worth explicitly noting that if "testable-cost-types" >>does not >>include values from "multi-cost-types", then those types can't be included in >>"constraints"/"or-constraints". [SR ] OK added this after 1st paragraph >> >>Please explain the default value for the index for the "constraints"/"or- >>constraints" express in this section in addition to where it is hidden in a note >>later in the document. [SR ] OK, moved text to example in section 5.4 >> >>Section 3.6.5 >> >>Uppercase for "must not" in the second paragraph. (The "may" later in the >>paragraph might be better as "can".) [SR ] OK, done. Also added an sentence in 1st paragraph to clarify it. >> >>In the example, the resource named "filtered-multicost-map" is provided for >>legacy reasons only. Why bother including "max-cost-types" and "cost-type- >>names" at all when "filtered-cost-map-extended" includes all that and more? [SR ] OK, before example, added text explaining when it is useful to specify both. Actually, "cost-type-names" is always present in filtered cost map capabilities. >> >>Section 4.1.1 >> >>The definition of the schema here (and later) actually redefines the object >>completely. I found that confusing initially. It would be good to identify the >>*changes* from the base specification somehow. [SR ] indeed. In RFC7285 {11.3.2.4}, member "cost-constraints" is not optional in object specification but is optional in the member description. Added a sentence at the end of 1st paragraph stressing the presence of the 2 new member listed in the beginning of this paragraph. Also added some text in introduction of section 4, on the notations, in particular "<m..n>". >> >>Can testable-cost-type-names be present and empty if cost-constraints is >>false? >>The first part of the definition permits that, the second forbids it. [SR ] The first part specifies member "testable-cost-type-names" as having "<1..*>" elements, if present. Added item to describe "cost-constraints" and moved text from "testable-cost-type-names" there. >> >>Section 4.1.2 >> >>The redefinition of PIDFilter is unnecessary. [SR ] OK, deleted >> >>IMPORTANT: pids is optional in RFC 7285. Why the change? [SR ] OK, corrected this, pids optional again. Thanks for catching this. >> >>I find the redefinition of the optionality of cost-type to be worthy of special >>note. [SR ] OK, added sentence in definition of "cost-type" member >> >>In the definition of "or-constraints", you use a "database query" >>where words >>would suffice. [SR ] OK, replaced "database query" with "words" >> >>Section 4.1.3 >> >>I find the choice of value for "cost-type" to be problematic. It is a string in the >>base protocol, so changing to an empty object is likely to cause more issues >>than simply omitting it. [SR ] Actually, "cost-type" is defined in {10.7} as an object unlike "cost-type-names" which is array of JSONString. >> >>Section 4.2 contains mismatched braces/parens for section references. [SR ] OK, corrected >> >>Section 4.2.2 >> >>IMPORTANT: This provides a definition for ReqFilteredCostMap that is very >>different to that in the base specification. I think that this should have been >>ReqEndpointCostMap. [SR ] OK, corrected this and replaced " ReqFilteredCostMap " with "ReqEndpointCostMap", thanks for catching this. >> >>As before, repeating the definition of EndpointFilter is unnecessary. [SR ] OK deleted >> >>Section 4.2.3 - see comment on 4.1.3 [SR ] OK, please see my answer. >> >>Section 5 >> >>I would be more comfortable if the examples used obviously-spurious >>metrics (e.g., "cattle-head-count", "smell", "shoe-size", etc...) than these >>metrics that are pretty plausible. More so when you claim that they are >>widely valued, which implies some sort of validity. [SR ] OK, replaced "hopcount" with "shoesize" and "bandwidthscore" with "sceneryrate" and adapted section introduction accordingly. >> >>It should be relatively easy to populate Content-Length now that the >>examples are "final". [SR ] OK, done >> >>Section 5.1 >> >>You have unmatched braces in "meta" due to the comment. [SR ] OK, corrected this. Also removed the comments in the IRD >> >>Section 5.2 >> >>Do you want to show one of the examples as having no cost values at all >>across all the cost types? [SR ] OK, updated text to say this only holds between PID2 and PID3 >>