----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15094/#review27895 -----------------------------------------------------------
Ship it! I like your idea for splitting out a public static routine in QueueFlowLimit that would determine the flow limits based on the current queue settings + the default values. I don't believe this would result in extra flow parameters appearing in the QMF object - I could be wrong, but I think the QMF object gets a copy of the settings map, which wouldn't be affected. But this is extra work. I think the current patch is fine, I just have two nits (see inline comments). /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp <https://reviews.apache.org/r/15094/#comment54312> Technically, resume == stop is permissible, though likely of little practical use. It's permissible as flow control kicks in when level > stop, and releases when level < resume. So there is no ambiguity when level == resume == stop (no change in flow state at this point). /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp <https://reviews.apache.org/r/15094/#comment54311> Just IMHO, the text should read: "configured flow limits are ignored"... or maybe "user-configured flow limits are ignored"... rather than "user flow limits are ignored"... I don't know what a "user flow limit" is. - Kenneth Giusti On Oct. 31, 2013, 12:51 p.m., Pavel Moravec wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/15094/ > ----------------------------------------------------------- > > (Updated Oct. 31, 2013, 12:51 p.m.) > > > Review request for qpid, Gordon Sim and Ted Ross. > > > Bugs: https://issues.apache.org/jira/browse/QPID-5278 > > https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/QPID-5278 > > > Repository: qpid > > > Description > ------- > > Trivial copy&paste bug fixed. Anyway I am not sure what the proper broker > behaviour in one of tested use cases: > > qpid-config add queue WrongQueue5 --max-queue-count=100 > --flow-resume-count=50 --durable > > should be. Currently, the flow-resume-count parameter is ignored as broker > logs: > > [Broker] info Queue "WrongQueue6": Flow limit created: flowStopCount=80, > flowResumeCount=70, flowStopSize=83886080, flowResumeSize=73400320 > > From the source code, I have a feeling this is intentional - flow control > defaults are overwritten only if flow-stop-[count|size] parameter is used. Am > I right? If so, then: > 1) there should be a warning - like the one in patch. > 2) queue settings (as provided in "qpid-config queues" output) should be > modified accordingly - currently it copy&paste the user settings that is then > ignored. That means, "qpid-config queues" provides wrong information. (this > is not covered by the patch) > > Any thoughts? > > > Diffs > ----- > > /trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp 1537398 > /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1537398 > /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1537398 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1537398 > /trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1537398 > > Diff: https://reviews.apache.org/r/15094/diff/ > > > Testing > ------- > > > Thanks, > > Pavel Moravec > >
