You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Pavel Moravec <pm...@redhat.com> on 2013/10/31 13:51:13 UTC
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
-----------------------------------------------------------
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.
Summary (updated)
-----------------
QPID-5278 & QPID-5281: Queue flow limit validation ignores size parameters & Creating a queue with invalid settings results in no queue but only its management object exists
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
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15094/#review29106
-----------------------------------------------------------
Ship it!
Nice work!
- Gordon Sim
On Nov. 19, 2013, 11:55 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15094/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2013, 11:55 a.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 1537924
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1537924
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1537924
> /trunk/qpid/cpp/src/tests/QueueFlowLimitTest.cpp 1537924
>
> Diff: https://reviews.apache.org/r/15094/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15094/#review29113
-----------------------------------------------------------
Ship it!
Ship It!
- Kenneth Giusti
On Nov. 19, 2013, 11:55 a.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15094/
> -----------------------------------------------------------
>
> (Updated Nov. 19, 2013, 11:55 a.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 1537924
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1537924
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1537924
> /trunk/qpid/cpp/src/tests/QueueFlowLimitTest.cpp 1537924
>
> Diff: https://reviews.apache.org/r/15094/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15094/
-----------------------------------------------------------
(Updated Nov. 19, 2013, 11:55 a.m.)
Review request for qpid, Gordon Sim and Ted Ross.
Changes
-------
New patch reflects Gordon's comments.
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 1537924
/trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1537924
/trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1537924
/trunk/qpid/cpp/src/tests/QueueFlowLimitTest.cpp 1537924
Diff: https://reviews.apache.org/r/15094/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15094/#review28021
-----------------------------------------------------------
/trunk/qpid/cpp/src/qpid/broker/QueueFactory.cpp
<https://reviews.apache.org/r/15094/#comment54480>
The problem with a raw pointer here is that if anything after this throws before we get to the observe() call, then the memory is leaked.
Would be nicer to have createLimit() return a shared_ptr, and then in observe() use boost::shared_from_this().
- Gordon Sim
On Nov. 1, 2013, 1:31 p.m., Pavel Moravec wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/15094/
> -----------------------------------------------------------
>
> (Updated Nov. 1, 2013, 1:31 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 1537924
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1537924
> /trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1537924
> /trunk/qpid/cpp/src/tests/QueueFlowLimitTest.cpp 1537924
>
> Diff: https://reviews.apache.org/r/15094/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Pavel Moravec
>
>
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
Posted by Pavel Moravec <pm...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/15094/
-----------------------------------------------------------
(Updated Nov. 1, 2013, 1:31 p.m.)
Review request for qpid, Gordon Sim and Ted Ross.
Changes
-------
Updated patch reflecting Gordon's comment (option 2 chosen) and Ken's idea from pseudo-code.
The patch changes behaviour in two cases:
$ qpid-config add queue MyQueue15 --durable --max-queue-count=100 --flow-resume-count=200
Failed: Exception: Exception from Agent: {u'error_code': 7, u'error_text': 'invalid-argument: Queue "MyQueue15": qpid.flow_resume_count=200 must be less or equal to qpid.flow_stop_count=80 (/data_xfs/qpid-trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp:52)'}
$
This in my opinion makes sense to prohibit (while unpatched broker allows such queue, silently ignoring user's flow-resume-count).
$ qpid-config add queue MyQueue27 --durable --flow-stop-size=90
Failed: Exception: Exception from Agent: {u'error_code': 7, u'error_text': 'invalid-argument: Queue "MyQueue27": qpid.flow_resume_size=73400320 must be less or equal to qpid.flow_stop_size=90 (/data_xfs/qpid-trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp:52)'}
$
Similar example where the patched broker follows flow setup rules just more strictly (note: 73400320 is (default) 70% of (default) 104857600 that is default-queue-limit).
BUT: with this patch, unit tests timeout without any obvious reason. Debugging it gives no clue to me:
ctest --force-new-ctest-process -vv --debug -R unit_test
..
/builddir/build/BUILD/cmake-2.6.4/Source/cmCTest.cxx:1185
0% 10 20 30 40 50 60 70 80 90 100%
|----|----|----|----|----|----|----|----|----|----|
(and now nothing).
Any idea what could be wrong? (I had to change cpp/src/tests/QueueFlowLimitTest.cpp to reflect QueueFlowLimit class API changes, cant be relevant?)
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 1537924
/trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1537924
/trunk/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1537924
/trunk/qpid/cpp/src/tests/QueueFlowLimitTest.cpp 1537924
Diff: https://reviews.apache.org/r/15094/diff/
Testing
-------
Thanks,
Pavel Moravec
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
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
>
>
Re: Review Request 15094: QPID-5278 & QPID-5281: Queue flow limit validation
ignores size parameters & Creating a queue with invalid settings results in
no queue but only its management object exists
Posted by Gordon Sim <gs...@redhat.com>.
-----------------------------------------------------------
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
>
>