Thanks Paul. We'll try to clarify those points in the text, so I won't comment in detail on your comments. I can see how your questions arise for a newcomer to GRASP, so they do need to be answered. It will take a little while to do this, so the AD will defer the draft to a later telechat.
Regards Brian Carpenter On 29-Oct-20 04:50, Paul Kyzivat wrote: > 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-anima-grasp-api-07 > Reviewer: Paul Kyzivat > Review Date: 2020-10-28 > IETF LC End Date: 2020-10-28 > IESG Telechat date: ? > > This draft is on the right track but has open issues, described in the > review. > > General: > > I began this review without any knowledge of Anima. I did read several > of the related documents for context, but my overall understanding > remains somewhat sketchy. So take my comments with with that in mind. > (At least you get a fresh, untainted perspective.) > > Issues: > > Major: 5 > Minor: 6 > Nits: 2 > > 1) MAJOR: Unclear model for API function usage > > As I read through sections 2.3.2 through 2.3.6 these I got progressively > more confused. I finally concluded that a big picture of how API > functions interact is lacking. > > For one thing, there isn't a clear delineation between those calls used > by requesters and those used by responders. And then valid sequencing of > calls is hard to tease out. > > It would be helpful to have one state model for requesters and another > for responders. It may also be helpful to break this out for threaded > and event loop variants. > > My subsequent issues regarding these sections are reflective of this > confusion. > > 2) MAJOR: What state does GRASP retain for Objectives? > > It is clear that the GRASP must retain the names of registered > objectives. There are implications that it must keep more. Seemingly it > must retain received flooded objectives, on a per-source basis. It is > unclear if it is only for registered objectives, or also for > unregistered objectives. This could potentially become a large resource > drain if there are lots of nodes flooding values. > > Negotiated objectives seem to be treated differently. It isn't clear to > me if GRASP needs to retain copies of their values. > > 3) MAJOR: Consistency of Objective definitions > > In section 2.3.1.2 and elsewhere, presumably all parties that use a > particular objective must agree on the values of synch, neg, dry, and > the format of the value. > > Apparently you are relying on each caller getting this right. What > happens when this isn't the case? How can blame be ascribed so that the > problem can be fixed? > > 4) MAJOR: Confusing semantics of 'request_negotiate' > > In section 2.3.4 I don't understand the following: > > 1. The 'session_nonce' parameter is null. In this case the > negotiation has succeeded immediately (the peer has > accepted the request). The returned 'proffered_objective' > contains the value accepted by the peer. > > IIUC this requires a network exchange with the peer. I don't see how > this can complete *immediately*. ISTM that this could only complete > immediately if it were satisfied from a local cache. That doesn't seem > appropriate for this function. > > Similarly, in bullet 2 I don't see how the proffered_objective would be > available in the initial call. > > Or is this text only applicable to a threaded implementation with > synchronous calls? > > Bullet 2 also says: > > ... The > returned 'proffered_objective' contains the first value > proffered by the negotiation peer. The contents of this > instance of the objective must be used in the subsequent > negotiation call because it contains the updated loop > count, sent by the negotiation peer. > > Do you mean this must be used in the call to negotiate_step? But that > would mean asking for what has already been granted. For the the > negotiation to be useful I would expect that the next round would ask > for a value somewhere between what was originally requested and what was > initially offered. > > I think you need to say more about the whole concept of negotiation and > how it is expected to work. Also, additional discussion of the purpose > and semantics of dry run negotiation. > > One more thing: you don't explain the semantics of 'timeout'. Is it only > used locally, to terminate the synchronous form of the call? Or is it > transmitted and used by the responder to control how long it might spend > trying to fulfill the request before giving up? > > 5) MAJOR: Confusing semantics of 'negotiate_step' > > In section 2.3.4, I'm confused by: > > Called in the same thread as the > preceding 'request_negotiate' or 'listen_negotiate' > > I understand use of this following request_negotiate, but not with > listen_negotiate. A negotiation should consist of an ordered sequence of > "rounds" of bidding. Allowing both requester and responder to initiate a > next step can lead to a disruption of the ordering. I would expect this > to only be used by the caller of request_negotiate. What am I missing? > > 6) MINOR(MAJOR?): Confusing semantics of 'negotiate_wait' > > Regarding section 2.3.4: I understand that the GRASP Confirm Waiting > message (from [I-D.ietf-anima-grasp]) and hence 'negotiate_wait' is only > used by the responder. How is the effect manifested in the requester? Is > a synchronous requester just silently extended? What about an event loop > requester? > > 7) MINOR(MAJOR?): Role of 'peer' in 'synchronize' > > The description of 'synchronize' (in section 2.3.5) says: > > - If the objective was already flooded, the flooded value is > returned immediately in the 'result' parameter. In this case, > the 'peer' and 'timeout' are ignored. > > Do you really want to ignore 'peer' in this case? > > I infer from the description of get_flood that the GRASP retains flooded > values separately for each flood source. So you should have the > capability to return only a value flooded by the requested peer. > > 8) MINOR(MAJOR?): Confusing semantics of 'flood' > > The descriptions of 'flood' in section 2.3.5 seem to imply that the > GRASP in each of the nodes receiving the flood saves the value, for use > in get_flood and synchronize. Is that right? (Actually, the description > of get_flood implies that flooded values are saved separately for every > flood source!) > > Are only registered objectives saved? Or must the GRASP save all the > values from every flood that it receives? > > What if one flood contains X, Y, and Z. Then a later flood from the same > source contains only X and Y. Presumably the new values of X and Y > replace the old ones. Does GRASP retain the value of Z, or discard it? > > ISTM (based on my limited understanding) that it would make more sense > for the GRASP to simply cache all objectives that have been registered > and/or received as a result of synchronization, together with their TTL. > And then subsequent requests would be satisfied from the cache as long > as the TTL hasn't expired. > > 9) MINOR: Clarify Session > > Section 2.2.3 implies that Session is a full fledged entity. It would be > helpful to flesh this out in more detail. E.g., describe it as a > distinct entity in the API and include a state model. > > 10) MINOR: Representing mutually exclusive items with booleans > > In section 2.3.1.3, 'synch' and 'neg' are mutually exclusive. By > representing them as independent booleans you introduce the possibility > of invalidly setting them both or neither. ISTM it would be better to > represent these as a single value (enumeration). > > A similar situation occurs in section 2.3.1.4. > > 11) MINOR: Managing thread for 'listen_negotiate' > > In section 2.3.4, suppose the ASA wants try to ensure that it always has > a listener ready for an incoming request. IIUC the only way to achieve > that is to create the maximum number of threads it is willing to devote > and set them all to listening. That is sub-optimal. It would be better > if it could somehow learn that another one is needed and create it. Some > additional API mechanisms would be needed to support that. > > 12) NIT: Idempotence > > Bullet #2 of section 2.2.1 says: > > To facilitate this, the API implementation would provide non- > blocking versions of all the functions that otherwise involve > blocking and queueing. In these calls, a 'noReply' code will be > returned by each call instead of blocking, until such time as the > event for which it is waiting (or a failure) has occurred. Thus, > for example, discover() would return 'noReply' instead of waiting > until discovery has succeeded or timed out. The discover() call > would be repeated in every cycle of the main loop until it > completes. Effectively, it becomes a polling call. > > This seems to contradict the earlier statement (in 2.2) that the calls > are not idempotent. Section 2.2.2 tries to clarify this, but it is not > very clear. > > 13) NIT: Confusing terminology in 'discover' > > In the description of 'discover()' (in section 2.3.3) the 'age_limit' > parameter is confusing. At the least the name seems wrong for the > described semantics. The implication is that an expiration date/time is > what GRASP stores, and that remaining time until expiration is what is > being used here. It is hard to see how it is appropriate to call that an > "age". ISTM that would better be called a TTL, and that is parameter > might then be called 'minimum_TTL'. > . > _______________________________________________ Gen-art mailing list Gen-art@ietf.org https://www.ietf.org/mailman/listinfo/gen-art
