----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/15094/#review27889 -----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp <https://reviews.apache.org/r/15094/#comment54309> While I understand what drove the addition of this name parameter, I don't really like it. Not a huge problem, but it seems less in keeping with the class as it is. Options would be: (a) to keep the flow config validation out of this method and make an explicit check on that in QueueFactory::create() (b) to reorganise the queue flow limit validation stuff to avoid needing the queue name when validating, and catch and throw a modified exception with the queue name added in on failure. I lean towards (b) but its a bit more work. - Gordon Sim 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 > >
