Joel, thanks for your review. Ingemar, thanks for addressing Joel’s comments. I have entered a No Objection ballot.
Alissa > On Oct 15, 2017, at 3:34 PM, Ingemar Johansson S > <[email protected]> wrote: > > Hi > Thanks for your review comments. Answers inline marked [IJ] > > /Ingemar > >> -----Original Message----- >> From: Joel Halpern [mailto:[email protected]] >> Sent: den 14 oktober 2017 21:11 >> To: [email protected] >> Cc: [email protected]; [email protected]; [email protected] >> Subject: Genart last call review of draft-ietf-rmcat-scream-cc-11 >> >> Reviewer: Joel Halpern >> Review result: Almost Ready >> >> 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-rmcat-scream-cc-11 >> Reviewer: Joel Halpern >> Review Date: 2017-10-14 >> IETF LC End Date: 2017-10-23 >> IESG Telechat date: 2017-10-26 >> >> Summary: This document is almost ready for publication as an experimental >> RFC. >> >> The reviewer hopes that the problem noted here are due to his mis-reading. >> >> Major issues: >> The description of "a" after the pseudocode in section 4.1.2 states that >> it is positive when qdelay is increasing, and negative when qdelay is >> decreasing. However, a is the ratio of two evaluations of the function R >> with different lags. And R is defined as the sum of products of entries >> in the history vector. The history elements (qdelay_fraction_avg) are >> always positive. So the terms in R are always positive. So a is always >> positive? > [IJ] You are absolutely right !, this is an error in the description. The > variable a should actually be computed based on the vector > qdelay_fraction_hist with the mean (DC component) removed. > The corrected code would be (I changed a to a_t as it is a temporary variable. > ===== > # Compute the average of the values in qdelay_fraction_hist > avg_t = average(qdelay_fraction_hist) > # R is an autocorrelation function of qdelay_fraction_hist, > # with the mean (DC component) removed, at lag K > # The subtraction of the scalar avg_t from qdelay_fraction_hist > # is performed element-wise. > a_t = R(qdelay_fraction_hist-avg_t,1)/R(qdelay_fraction_hist-avg_t,0) > . > . > ===== > >> >> Minor issues: >> There appears to be something odd with the mathematical expression for >> the >> autocorrelation function R or its usage. As written, with lag k the >> function uses N-k terms. Which means that if the qdelay stays perfectly >> constant, a will be N-1/N rather than 1 (i.e. 0.95). If this is >> intentional, it would be good to say so. > [IJ] I believe this I becomes clear with the correction above. It is possible > to compute an unbiased estimate but it is not necessary for the function > here. > >> >> What does the text in section 3.1 reading "It is however necessary that >> they [ sender and receiver] use the same clock frequency" mean? Times >> are >> exchanged. Frequencies are not. Is this intended to be a statement >> about >> resolution? it appears to describe something that is not visible to the >> protocol. As such, what happens if the requirement is not met and the >> failure is not detected?" > [IJ] OK. What about add something like > "Failure to meet this requirement leads to malfunction in the SCReAM > congestion control algorithm as the queue delay will not be estimated > correctly." > >> >> max_bytes_in_flight appears in the pseudocode in section 4.1.2.2, but >> does >> not appear in the list of state variables earlier in the document. > [IJ] OK, will fix this >> >> Nits/editorial comments: >> In the pseudocode in section 4.1.2, is the variable "a" really "a_t"? > [IJ] Should be a_t >> >> In section 4.1.1.2 in the definition of min_cwnd, should there be a note >> about the (admittedly unlikely) case where 2*MSS is less then >> MIN_CWND? > [IJ] actually min_cwnd should be replaced by MIN_CWND and min_cwnd should be > removed from the document >> >> In the last paragraph of 4.1.2.2, is the new cwnd limited by the variable >> min_cwnd as stated in the text, or the constant MIN-CWND as shown in >> the >> pseudocode? > [IJ] Should be MIN_CWND in all places. min_cwnd should be removed. >> > > _______________________________________________ > Gen-art mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/gen-art _______________________________________________ Gen-art mailing list [email protected] https://www.ietf.org/mailman/listinfo/gen-art
