-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15094/
-----------------------------------------------------------
(Updated Oct. 31, 2013, 12:50 p.m.)
Review request for qpid, Gordon Sim and Ted Ross.
Changes
-------
Updated patch fixing also QPID-5281 (and one another subtle bug).
Due to QPID-5281, I didn't incorporate Ken's idea - see below.
Comments to the patch:
1)
-void QueueSettings::validate() const
+void QueueSettings::validate(const std::string& name) const
is due to necessity to print out queue name in QueueFlowLimit::verify, in case
a limit is wrongly set up.
2)
in validateFlowConfig:
- if (resume > stop) {
+ if ((stop) && (resume >= stop)) {
fixes the case when stop == resume != 0 (should raise exception).
3) QueueFlowLimit::verify method fixes QPID-5281.
Broker behaviour with the patch is fine: it allows all queues with proper
config and denies all with wrong one. BUT I don't like that currently verifying
is done on one place, and setting the flow limits on another - some code is
duplicit, and if some flow control logic is changed, two places needs to be
updated. That prevented me from applying Ken's idea.
Gotchas in attempting to unify "verify&set" approach:
- it makes sense to call QueueFlowLimit::verify within QueueSettings::validate,
where all other validations are made
- but that is executed before QueueFlowLimit constructor, hence verify is static
- that prevents unification with QueueFlowLimit::setDefaults and mainly with
createLimit
- an alternative approach in filling QueueSettings (flowStop and flowResume) in
a static method also by default values would cause QMF object would be created
with _updated_ queue settings, i.e. every queue will have full list of flow
variables - imho not desired
Simply, I dont see a simple way of unification the code. The patch as is fixes
both (all mentioned) bugs and does not have any known side effects.
FYI I used this script for testing various queues' setups:
qNr=0
set_var() {
if [ "$1" == "no" ]; then
echo ""
else
echo "$2=$1"
fi
}
run_qpid_config() {
qNr=$(($((qNr))+1))
name="MyQueue${qNr}"
qCount=$(set_var ${1} "--max-queue-count")
fStopCout=$(set_var ${2} "--flow-stop-count")
fResCount=$(set_var ${3} "--flow-resume-count")
qSize=$(set_var ${4} "--max-queue-size")
fStopSize=$(set_var ${5} "--flow-stop-size")
fResumeSize=$(set_var ${6} "--flow-resume-size")
cmd="qpid-config add queue ${name} --durable ${qCount} ${fStopCout}
${fResCount} ${qSize} ${fStopSize} ${fResumeSize}"
echo "$cmd"
$cmd
}
# qCount fStopCount fResumeCount qSize fStopSize fResumeSize
run_qpid_config 100 90 80 1000 900 800
run_qpid_config 100 90 80 "no" "no" "no"
run_qpid_config "no" "no" "no" 1000 900 800
run_qpid_config 100 90 "no" "no" "no" "no"
run_qpid_config 100 "no" 80 "no" "no" "no"
run_qpid_config "no" 90 80 "no" "no" "no"
run_qpid_config "no" "no" 80 "no" "no" "no"
run_qpid_config "no" 90 "no" "no" "no" "no"
run_qpid_config 100 "no" "no" "no" "no" "no"
run_qpid_config "no" "no" "no" "no" "no" "no"
run_qpid_config 100 200 "no" "no" "no" "no"
run_qpid_config 100 200 80 "no" "no" "no"
run_qpid_config 100 200 150 "no" "no" "no" "no"
run_qpid_config 100 200 250 "no" "no" "no" "no"
run_qpid_config 100 "no" 200 "no" "no" "no"
run_qpid_config 100 90 200 "no" "no" "no"
run_qpid_config "no" 100 200 "no" "no" "no"
run_qpid_config "no" 0 "no" "no" "no" "no"
run_qpid_config "no" 0 80 "no" "no" "no"
run_qpid_config 100 0 80 "no" "no" "no"
run_qpid_config 100 0 "no" "no" "no" "no"
run_qpid_config 100 0 200 "no" "no" "no"
run_qpid_config "no" "no" "no" 100 90 "no" "no" "no" "no"
run_qpid_config "no" "no" "no" 100 "no" 80 "no" "no" "no"
run_qpid_config "no" "no" "no" "no" 90 80 "no" "no" "no"
run_qpid_config "no" "no" "no" "no" "no" 80 "no" "no" "no"
run_qpid_config "no" "no" "no" "no" 90 "no" "no" "no" "no"
run_qpid_config "no" "no" "no" 100 "no" "no" "no" "no" "no"
run_qpid_config "no" "no" "no" "no" "no" "no" "no" "no" "no"
run_qpid_config "no" "no" "no" 100 200 "no" "no" "no" "no"
run_qpid_config "no" "no" "no" 100 200 80 "no" "no" "no"
run_qpid_config "no" "no" "no" 100 200 150 "no" "no" "no" "no"
run_qpid_config "no" "no" "no" 100 200 250 "no" "no" "no" "no"
run_qpid_config "no" "no" "no" 100 "no" 200 "no" "no" "no"
run_qpid_config "no" "no" "no" 100 90 200 "no" "no" "no"
run_qpid_config "no" "no" "no" "no" 100 200 "no" "no" "no"
run_qpid_config "no" "no" "no" "no" 0 "no" "no" "no" "no"
run_qpid_config "no" "no" "no" "no" 0 80 "no" "no" "no"
run_qpid_config "no" "no" "no" 100 0 80 "no" "no" "no"
run_qpid_config "no" "no" "no" 100 0 "no" "no" "no" "no"
run_qpid_config "no" "no" "no" 100 0 200 "no" "no" "no"
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 (updated)
-----
/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
