You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Ted Ross <tr...@apache.org> on 2013/03/08 16:13:08 UTC
Review Request: Change queue-depth threshold alerts to be crossing-triggered
rather than level-sensitive.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9824/
-----------------------------------------------------------
Review request for qpid.
Description
-------
Change the queue-depth threshold alerts to be edge-triggered rather than level-sensitive. Provide upper and lower thresholds for both message count and byte count. Two events are generated: "Crossing-Upward" events are raised when the upper threshold is crossed lower-to-higher and "Crossing-Downward" events are raised when the lower threshold is crossed higher-to-lower.
This removes the need for a mechanism to throttle events and it also removes the need for special handling for recursion (thresholds-on-the-event-queue).
If the lower thresholds are not specified or invalid, they are set to half of the upper threshold.
This addresses bug QPID-4632.
https://issues.apache.org/jira/browse/QPID-4632
Diffs
-----
trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1454413
trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1454413
trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1454413
trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1454413
trunk/qpid/specs/management-schema.xml 1454413
trunk/qpid/tests/src/py/qpid_tests/broker_0_10/threshold.py 1454413
Diff: https://reviews.apache.org/r/9824/diff/
Testing
-------
The existing tests were modified and a new test for threshold hysteresis was added.
Thanks,
Ted Ross
Re: Review Request: Change queue-depth threshold alerts to be
crossing-triggered rather than level-sensitive.
Posted by Ted Ross <tr...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9824/#review17610
-----------------------------------------------------------
I was surprised to see that this patch doesn't break the Java Broker build. Does the Java Broker not implement the queue threshold event?
- Ted Ross
On March 8, 2013, 3:13 p.m., Ted Ross wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9824/
> -----------------------------------------------------------
>
> (Updated March 8, 2013, 3:13 p.m.)
>
>
> Review request for qpid.
>
>
> Description
> -------
>
> Change the queue-depth threshold alerts to be edge-triggered rather than level-sensitive. Provide upper and lower thresholds for both message count and byte count. Two events are generated: "Crossing-Upward" events are raised when the upper threshold is crossed lower-to-higher and "Crossing-Downward" events are raised when the lower threshold is crossed higher-to-lower.
>
> This removes the need for a mechanism to throttle events and it also removes the need for special handling for recursion (thresholds-on-the-event-queue).
>
> If the lower thresholds are not specified or invalid, they are set to half of the upper threshold.
>
>
> This addresses bug QPID-4632.
> https://issues.apache.org/jira/browse/QPID-4632
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1454413
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1454413
> trunk/qpid/specs/management-schema.xml 1454413
> trunk/qpid/tests/src/py/qpid_tests/broker_0_10/threshold.py 1454413
>
> Diff: https://reviews.apache.org/r/9824/diff/
>
>
> Testing
> -------
>
> The existing tests were modified and a new test for threshold hysteresis was added.
>
>
> Thanks,
>
> Ted Ross
>
>
Re: Review Request: Change queue-depth threshold alerts to be
crossing-triggered rather than level-sensitive.
Posted by Ted Ross <tr...@apache.org>.
> On March 8, 2013, 5:02 p.m., Chug Rolke wrote:
> > trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp, line 214
> > <https://reviews.apache.org/r/9824/diff/1/?file=268374#file268374line214>
> >
> > How about a test that the new thresholds have the down limit at least one (or N?) below the up limit? If ever down > up you get an event storm.
It's already in there (lines 84, 85 of ThresholdAlerts.cpp). If the lower threshold is zero or greater/equal to the upper threshold, it's set to the upper threshold divided by two.
- Ted
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9824/#review17614
-----------------------------------------------------------
On March 8, 2013, 3:13 p.m., Ted Ross wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9824/
> -----------------------------------------------------------
>
> (Updated March 8, 2013, 3:13 p.m.)
>
>
> Review request for qpid.
>
>
> Description
> -------
>
> Change the queue-depth threshold alerts to be edge-triggered rather than level-sensitive. Provide upper and lower thresholds for both message count and byte count. Two events are generated: "Crossing-Upward" events are raised when the upper threshold is crossed lower-to-higher and "Crossing-Downward" events are raised when the lower threshold is crossed higher-to-lower.
>
> This removes the need for a mechanism to throttle events and it also removes the need for special handling for recursion (thresholds-on-the-event-queue).
>
> If the lower thresholds are not specified or invalid, they are set to half of the upper threshold.
>
>
> This addresses bug QPID-4632.
> https://issues.apache.org/jira/browse/QPID-4632
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1454413
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1454413
> trunk/qpid/specs/management-schema.xml 1454413
> trunk/qpid/tests/src/py/qpid_tests/broker_0_10/threshold.py 1454413
>
> Diff: https://reviews.apache.org/r/9824/diff/
>
>
> Testing
> -------
>
> The existing tests were modified and a new test for threshold hysteresis was added.
>
>
> Thanks,
>
> Ted Ross
>
>
Re: Review Request: Change queue-depth threshold alerts to be
crossing-triggered rather than level-sensitive.
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9824/#review17614
-----------------------------------------------------------
trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp
<https://reviews.apache.org/r/9824/#comment37361>
How about a test that the new thresholds have the down limit at least one (or N?) below the up limit? If ever down > up you get an event storm.
- Chug Rolke
On March 8, 2013, 3:13 p.m., Ted Ross wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9824/
> -----------------------------------------------------------
>
> (Updated March 8, 2013, 3:13 p.m.)
>
>
> Review request for qpid.
>
>
> Description
> -------
>
> Change the queue-depth threshold alerts to be edge-triggered rather than level-sensitive. Provide upper and lower thresholds for both message count and byte count. Two events are generated: "Crossing-Upward" events are raised when the upper threshold is crossed lower-to-higher and "Crossing-Downward" events are raised when the lower threshold is crossed higher-to-lower.
>
> This removes the need for a mechanism to throttle events and it also removes the need for special handling for recursion (thresholds-on-the-event-queue).
>
> If the lower thresholds are not specified or invalid, they are set to half of the upper threshold.
>
>
> This addresses bug QPID-4632.
> https://issues.apache.org/jira/browse/QPID-4632
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1454413
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1454413
> trunk/qpid/specs/management-schema.xml 1454413
> trunk/qpid/tests/src/py/qpid_tests/broker_0_10/threshold.py 1454413
>
> Diff: https://reviews.apache.org/r/9824/diff/
>
>
> Testing
> -------
>
> The existing tests were modified and a new test for threshold hysteresis was added.
>
>
> Thanks,
>
> Ted Ross
>
>
Re: Review Request: Change queue-depth threshold alerts to be
crossing-triggered rather than level-sensitive.
Posted by Chug Rolke <cr...@redhat.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9824/#review17617
-----------------------------------------------------------
Ship it!
- Chug Rolke
On March 8, 2013, 3:13 p.m., Ted Ross wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9824/
> -----------------------------------------------------------
>
> (Updated March 8, 2013, 3:13 p.m.)
>
>
> Review request for qpid.
>
>
> Description
> -------
>
> Change the queue-depth threshold alerts to be edge-triggered rather than level-sensitive. Provide upper and lower thresholds for both message count and byte count. Two events are generated: "Crossing-Upward" events are raised when the upper threshold is crossed lower-to-higher and "Crossing-Downward" events are raised when the lower threshold is crossed higher-to-lower.
>
> This removes the need for a mechanism to throttle events and it also removes the need for special handling for recursion (thresholds-on-the-event-queue).
>
> If the lower thresholds are not specified or invalid, they are set to half of the upper threshold.
>
>
> This addresses bug QPID-4632.
> https://issues.apache.org/jira/browse/QPID-4632
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1454413
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1454413
> trunk/qpid/specs/management-schema.xml 1454413
> trunk/qpid/tests/src/py/qpid_tests/broker_0_10/threshold.py 1454413
>
> Diff: https://reviews.apache.org/r/9824/diff/
>
>
> Testing
> -------
>
> The existing tests were modified and a new test for threshold hysteresis was added.
>
>
> Thanks,
>
> Ted Ross
>
>
Re: Review Request: Change queue-depth threshold alerts to be
crossing-triggered rather than level-sensitive.
Posted by Kenneth Giusti <kg...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/9824/#review17616
-----------------------------------------------------------
Ship it!
Aside from the minor nits below, looks good.
Removing the existing threshold event and config will be visible externally. Should it be left in as deprecated for one release cycle?
trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp
<https://reviews.apache.org/r/9824/#comment37363>
Overriding a bad value (down > up) with a valid default is good, but it's probably a good idea to drop a log message when doing so.
trunk/qpid/tests/src/py/qpid_tests/broker_0_10/threshold.py
<https://reviews.apache.org/r/9824/#comment37364>
what happens if depth toggles between 9 and 10? It appears the code would prevent generating events. This test should explicitly trigger that case to ensure the code doesn't get broken over time. [and also toggle between 4 and 5 to cover the down event]
- Kenneth Giusti
On March 8, 2013, 3:13 p.m., Ted Ross wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/9824/
> -----------------------------------------------------------
>
> (Updated March 8, 2013, 3:13 p.m.)
>
>
> Review request for qpid.
>
>
> Description
> -------
>
> Change the queue-depth threshold alerts to be edge-triggered rather than level-sensitive. Provide upper and lower thresholds for both message count and byte count. Two events are generated: "Crossing-Upward" events are raised when the upper threshold is crossed lower-to-higher and "Crossing-Downward" events are raised when the lower threshold is crossed higher-to-lower.
>
> This removes the need for a mechanism to throttle events and it also removes the need for special handling for recursion (thresholds-on-the-event-queue).
>
> If the lower thresholds are not specified or invalid, they are set to half of the upper threshold.
>
>
> This addresses bug QPID-4632.
> https://issues.apache.org/jira/browse/QPID-4632
>
>
> Diffs
> -----
>
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.h 1454413
> trunk/qpid/cpp/src/qpid/broker/QueueSettings.cpp 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1454413
> trunk/qpid/cpp/src/qpid/broker/ThresholdAlerts.cpp 1454413
> trunk/qpid/specs/management-schema.xml 1454413
> trunk/qpid/tests/src/py/qpid_tests/broker_0_10/threshold.py 1454413
>
> Diff: https://reviews.apache.org/r/9824/diff/
>
>
> Testing
> -------
>
> The existing tests were modified and a new test for threshold hysteresis was added.
>
>
> Thanks,
>
> Ted Ross
>
>