LGTM On Oct 5, 2016 3:25 AM, "Joel M. Halpern" <[email protected]> wrote:
> We probably should tweak the wording on REQ-12. The notification is only > needed when the new operation succeeds. > When the new operation fails, the requester will receive an error, and the > original state is still there, so no notification is needed. I should have > realized that in my earlier review. > > Suggested fix, add text at left margin: > Ephemeral-REQ-12: When a collision occurs as two clients are trying > to write the same data node, this collision is considered an error > and priorities were created to give a deterministic result. When > there is a collision, > and the data node is changed, > a notification (which includes indicating data > node the collision occurred on) MUST BE sent to the original client > to give the original client a chance to deal with the issues > surrounding the collision. The original client may need to fix their > state. > > Yours, > Joel > > On 10/4/16 10:37 PM, Alia Atlas wrote: > >> Hi, >> >> As is customary, I have done my AD review >> of draft-ietf-i2rs-ephemeral-state-18. First, I would like to thank Sue >> and Jeff for their hard work pulling this document together in an effort >> to add clarity to the requirements. >> >> I do have a number of comments - many relatively minor. Assuming a fast >> turn-around, as usual from I2RS, we should be able to have this on the >> Oct 27 telechat - which will mean it needs to enter IETF Last Call >> before the end of this week. >> >> Here is my review: >> >> Major: >> >> 1) Ephemeral-REQ-12: This specifies that a notification be sent to the >> original client, regardless of whether it won or lost the priority >> collision. >> I had assumed that the notification would go to either the requesting >> client >> or the original client depending on which one lost the priority >> comparison. >> I have some concerns about an indirect flood of notifications caused by a >> requesting client that has the lower priority. Regardless, clarifying >> that >> the lower-priority client is notified is important. >> >> >> >> Minor: >> a) Intro: Remove "3 suggest protocol strawman" as something that >> the I2RS requirements must do. I know that is how the process >> has been working out - but it isn't dictated by the technology >> at all - as the other 2 are. Similarly, replace the following >> paragraph "The purpose of these requirements and the suggested >> protocol strawman is to provide a quick turnaround on creating >> the I2RS protocol." with something like "The purpose of these >> requirements is to ensure clarity during I2RS protocol creation." >> >> b) Section 2: "The following are ten requirements that [RFC7921] >> contains which provide context for the ephemeral data state >> requirements given in sections 3-8:" >> How about "The following are requirements distilled from [RFC7921] >> that provide context for..." >> >> 1) Not relevant for ephemeral - this matters for pub/sub (suggest >> removal) >> 2) Relevant for ephemeral b/c of vague performance requirements on >> possible solutions. >> 3) What changes if the data model is protocol dependent? Is this >> just that >> the model may be an augmentation/extension of an existing module? >> 4) Absolutely - keep >> 5) Absolutely - keep >> 6) Remove - not relevant for ephemeral just security requirements >> 7) Remove - not relevant for ephemeral just security requirements >> 8) Absolutely - keep (but says storing secondary identity on >> deletion when >> that isn't mentioned for (4) b/c it's about priority - so >> clarify slightly) >> 9) Absolutely - keep >> 10) Remove - not relevant for ephemeral >> >> c) Sec 3.3 bullet 2: This refers to YANG data model instead of YANG >> module as >> in bullet 1. If there's a reason for the difference, please clarify >> and otherwise >> make consistent. >> >> d) Sec 5 & 6 for NETCONF and RESTCONF are the same requirements. Please >> consolidate into a section of "The changes to NETCONF and the conceptual >> changes to RESTCONF are" >> >> e) Sec 8: I found this pull-out unclear. "multiple operations in one >> or more messages; though errors in >> message or operation will have no effect on other messages or >> commands even they are related." >> >> I think you mean "Multiple operations in one message can be sent. >> However >> an error in one operation MUST NOT stop additional operations from >> being >> carried out nor can it cause previous operations in the same message >> to >> be rolled back." >> >> Nits: >> >> i) Abstract: "attempting to meet I2RS needs has to provide"/ >> "attempting to meet the needs of I2RS has to provide" >> >> ii) 3.2: "MPLS LSP-ID or BGP IN-RIB" please expand acronyms >> >> iii) Sec 5 last sentence: Either missing a ( or has an unneeded ). >> >> iv) Ephemeral-REQ-11: "I2RS Protocol I2RS Protocol" repeated >> >> >> >> _______________________________________________ >> i2rs mailing list >> [email protected] >> https://www.ietf.org/mailman/listinfo/i2rs >> >>
_______________________________________________ i2rs mailing list [email protected] https://www.ietf.org/mailman/listinfo/i2rs
