[ https://issues.apache.org/jira/browse/QPID-7618?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15876286#comment-15876286 ]
Lorenz Quack edited comment on QPID-7618 at 2/22/17 11:44 AM: -------------------------------------------------------------- Looking at Alex's patch: * In {{Queue_logmessages.property}} you reused {{QUE-1003/4}} for {{OVER-/UNDERFULL_MESSAGES}}. Is there precedence for using the same operational log ids for multiple messages even if they are closely related? * In {{Queue.java}} regarding {{queue.queueFlowResumeLimit}} ** The description is a bit awkward. I think {{Percentage}} instead of {{Number of percents}} would be better. ** I hope this also applies to {{maximumQueueDepthMessages}} and not only {{maximumQueueDepthBytes}}. ** This sets {{DEFAULT_FLOW_CONTROL_RESUME_LIMIT}} to 80% instead of the 50% suggested by Keith. I tend to lean more towards 80% than 50%. I just wanted to point it out. ** This implements the resume threshold as a percentage for both bytes and messages as Keith suggested and not as two separate context variables for bytes and messages as suggested by Tomas. I don't have a strong feeling about this either way. * The changes in {{Queue.java}} break backwards compatibility and should be documented as such for the next release. This also means this should not go into 6.x. Interesting question: Do we need a versioning scheme for context variables? * {{DEFAULT_MAXIMUM_QUEUE_DEPTH_MESSAGES = 0;}} so by default we do not allow messages on a queue? That seems unfortunate. Same for {{_BYTES}} * IMHO, {{maximumQueueDepth* == 0}} should mean no messages allowed on the queue. Currently means unlimited. IMHO, we should either set it to {{LONG_MAX}} or to {{< 0}} if we want to indicate unlimited. In the RingOverflowPolicy the state is even more unfortunate. There, when both message and bytes limit are set to {{0}} it means do not accept any messages but if only one is {{0}} and the other is {{> 0}} the {{0}} indicates to not take that limit into account. * In the description of {{DEFAULT_MAXIMUM_QUEUE_DEPTH_\*}} and {{getMaximumQueueDepth\*}} I would use "number" rather than "count" and "amount" but then again I am not a native English speaker. * {{AbstractQueue}} ** {{onValidate}} was removed and {{validateChange}} changed. They seemed to perform useful checks. The equivalent check now would be that {{queue.queueFlowResumeLimit < 100}}, no? ** It seems surprising that {{ServerMessage#isResourceAcceptable}} always returns true on 0-8 through 0-10 while on 1.0 it checks for delivery delay. Is that a bug? ** In {{AbstractQueue#route}} the {{if}} branch {{if (!_overflowPolicyHandler.isAcceptable(message, result)) ; // noop}} seems silly. The reason is the convention that methods like {{isSomething}}, {{hasSomething}}, {{getSomething}} should not have side effects. I think we should adhere to that convention. * I think {{ProducerFlowControlOverflowPolicyHandler}} is unsafely publishing a reference to {{this}} in the constructor by passing {{_changeListener}} (which AFAIK has an implicit reference to the enclosing instance) to {{Queue#addChangeListener}}. * In {{AbstractQueueEntryList#updateStatsOnStateChange}} I don't think the call to {{_queue.checkCapacity();}} belongs there. The method is updating queue statistics. * Typo: {{ProducerFlowControlOverflowPolicyHandler#unblockIfUndefull}} and the local variable {{bytesUndefull}} in that method are missing the "r" in "Unde *r* full" * It surprises me that {{ProducerFlowControlOverflowPolicyHandler#checkCapacity}} only checks to unblock the flow. should it not also check to see whether it needs to block it? * The way the logic is currently structured in {{ProducerFlowControlOverflowPolicyHandler#unblockIfUndefull}} you can unblock the flow without seeing a corresponding operational log message. This is undesireable. This occurrs when maximumQueueDepthBytes is set to {{0}} while the flow is blocked. * {{queueFlowResumeRatio}} in {{ProducerFlowControlOverflowPolicyHandler$AbstractConfigurationChangeListener.attributeSet()}} is not a "ratio"; it is a percentage. Same goes for {{ProducerFlowControlTest#getFlowResumeRatio}} * The construction of the AccessControllerContext in {{ProducerFlowControlOverflowPolicyHandler#handleOverflow}} is expensive and only needed if an overflow occurred. Therefore I would switch around the if blocks to first check for overflow and then for the session on the Subject. * We could see multiple OVERFULL oerational log messages for a single event if there are multiple enqueue attempts before the flow control is enacted. I think this is undesirable and preventable by wrapping the inner part of {{ProducerFlowControlOverflowPolicyHandler#handleOverflow}} in a {{if (_overfull.compareAndSet(false, true))}} block. * I think there is a race between blocking and unblocking. We enter the blocking code past the {{_overfull.set(true)}} then a message gets dequeued and we call {{unblockIfUndefull}} which will log the operational log messages of UNDERFULL and call unblock even though the blocking code has not yet executed. Then the other enqueuing thread resumes logs the OVERFULL messages and blocks the session. the subsequent code then logs UNDERFULL again and unblocks the session. I am sure there are other unpleasant scenarios possible. * In {{QueueArgumentsConverter#convertWireArgsToModel}} this line {{context.put(Queue.QUEUE_FLOW_RESUME_LIMIT, new DecimalFormat("#.00").format(Math.round(ratio * 10000.0) / 100.0));}} looks curious. What is the advantage of using it instead of the more common {{String.format("%.2f", ratio * 100)}}? {{DecimalFormat}} is also used in {{VirtualHostStoreUpgraderAndRecoverer}} and {{ProducerFlowControlTest#getFlowResumeRatio}}. * In {{RingOverflowPolicyHandler}} I am curious why you use {{_queue.getEntries() instanceof PriorityQueueList}} instead of {{_queue instanceof PriorityQueue}}? * I am not sure whether the current behaviour of rejecting messages if they can't go on the RingQueue is the right one. Imagine the message limit is {{1}} and we send a message "A" of priority {{2}} concurrently with a message "B" of priority {{1}}. Now the result the application publishing "B" sees is racey (either accept or reject) while the result on the broker is identical (only message "B" is on the queue). That seems odd to me. Similar arguments apply to the other checks performed in {{RingOverflowPolicyHandler#isAcceptable}}. I'm basically arguing to kill that method. * I don't like the speciallisation for {{PriorityQueue}} in {{RingOverflowPolicyHandler#handleOverflow}}. Maybe if we had a {{Queue#getLast()}} method. A further argument in support of this is that I think the current implementation is not correct for {{SortedQueue}}. * In the Upgrader I think we should fail if we cannot upgrade something successfully instead of printing a warning and using the default. * In {{AbstractQueueTestBase}} resetting the queue to its default values at the end of the tests is not necessary because the queue is recreated before every test in the test {{setUp}} method. * The FlowControlPolicy is not tested. * Changing of attributes while messages on the queue and/or flow control enforced not tested * In 0-10, in {{ServerSession}} the check {{if (result.isRoutingFailure())}} was removed. Should it have been moved to the {{ServerSessionDelegate}} along with the initial creation of the {{routingResult}}? * In the UI I think the description of "Maximum Queue Depth (Bytes)" is ambigious. "Maximum size of messages (including header) in the queue" could mean the total size of messages on the queue or the size of each individual message on the queue. was (Author: lorenz.quack): Looking at Alex's patch: * In {{Queue_logmessages.property}} you reused {{QUE-1003/4}} for {{OVER-/UNDERFULL_MESSAGES}}. Is there precedence for using the same operational log ids for multiple messages even if they are closely related? * In {{Queue.java}} regarding {{queue.queueFlowResumeLimit} ** The description is a bit awkward. I think {{Percentage}} instead of {{Number of percents}} would be better. ** I hope this also applies to {{maximumQueueDepthMessages}} and not only {{maximumQueueDepthBytes}}. ** This sets {{DEFAULT_FLOW_CONTROL_RESUME_LIMIT}} to 80% instead of the 50% suggested by Keith. I tend to lean more towards 80% than 50%. I just wanted to point it out. ** This implements the resume threshold as a percentage for both bytes and messages as Keith suggested and not as two separate context variables for bytes and messages as suggested by Tomas. I don't have a strong feeling about this either way. * The changes in {{Queue.java}} break backwards compatibility and should be documented as such for the next release. This also means this should not go into 6.x. Interesting question: Do we need a versioning scheme for context variables? * {{DEFAULT_MAXIMUM_QUEUE_DEPTH_MESSAGES = 0;}} so by default we do not allow messages on a queue? That seems unfortunate. Same for {{_BYTES}} * IMHO, {{maximumQueueDepth* == 0}} should mean no messages allowed on the queue. Currently means unlimited. IMHO, we should either set it to {{LONG_MAX}} or to {{< 0}} if we want to indicate unlimited. In the RingOverflowPolicy the state is even more unfortunate. There, when both message and bytes limit are set to {{0}} it means do not accept any messages but if only one is {{0}} and the other is {{> 0}} the {{0}} indicates to not take that limit into account. * In the description of {{DEFAULT_MAXIMUM_QUEUE_DEPTH_\*}} and {{getMaximumQueueDepth\*}} I would use "number" rather than "count" and "amount" but then again I am not a native English speaker. * {{AbstractQueue}} ** {{onValidate}} was removed and {{validateChange}} changed. They seemed to perform useful checks. The equivalent check now would be that {{queue.queueFlowResumeLimit < 100}}, no? ** It seems surprising that {{ServerMessage#isResourceAcceptable}} always returns true on 0-8 through 0-10 while on 1.0 it checks for delivery delay. Is that a bug? ** In {{AbstractQueue#route}} the {{if}} branch {{if (!_overflowPolicyHandler.isAcceptable(message, result)) ; // noop}} seems silly. The reason is the convention that methods like {{isSomething}}, {{hasSomething}}, {{getSomething}} should not have side effects. I think we should adhere to that convention. * I think {{ProducerFlowControlOverflowPolicyHandler}} is unsafely publishing a reference to {{this}} in the constructor by passing {{_changeListener}} (which AFAIK has an implicit reference to the enclosing instance) to {{Queue#addChangeListener}}. * In {{AbstractQueueEntryList#updateStatsOnStateChange}} I don't think the call to {{_queue.checkCapacity();}} belongs there. The method is updating queue statistics. * Typo: {{ProducerFlowControlOverflowPolicyHandler#unblockIfUndefull}} and the local variable {{bytesUndefull}} in that method are missing the "r" in "Unde *r* full" * It surprises me that {{ProducerFlowControlOverflowPolicyHandler#checkCapacity}} only checks to unblock the flow. should it not also check to see whether it needs to block it? * The way the logic is currently structured in {{ProducerFlowControlOverflowPolicyHandler#unblockIfUndefull}} you can unblock the flow without seeing a corresponding operational log message. This is undesireable. This occurrs when maximumQueueDepthBytes is set to {{0}} while the flow is blocked. * {{queueFlowResumeRatio}} in {{ProducerFlowControlOverflowPolicyHandler$AbstractConfigurationChangeListener.attributeSet()}} is not a "ratio"; it is a percentage. Same goes for {{ProducerFlowControlTest#getFlowResumeRatio}} * The construction of the AccessControllerContext in {{ProducerFlowControlOverflowPolicyHandler#handleOverflow}} is expensive and only needed if an overflow occurred. Therefore I would switch around the if blocks to first check for overflow and then for the session on the Subject. * We could see multiple OVERFULL oerational log messages for a single event if there are multiple enqueue attempts before the flow control is enacted. I think this is undesirable and preventable by wrapping the inner part of {{ProducerFlowControlOverflowPolicyHandler#handleOverflow}} in a {{if (_overfull.compareAndSet(false, true))}} block. * I think there is a race between blocking and unblocking. We enter the blocking code past the {{_overfull.set(true)}} then a message gets dequeued and we call {{unblockIfUndefull}} which will log the operational log messages of UNDERFULL and call unblock even though the blocking code has not yet executed. Then the other enqueuing thread resumes logs the OVERFULL messages and blocks the session. the subsequent code then logs UNDERFULL again and unblocks the session. I am sure there are other unpleasant scenarios possible. * In {{QueueArgumentsConverter#convertWireArgsToModel}} this line {{context.put(Queue.QUEUE_FLOW_RESUME_LIMIT, new DecimalFormat("#.00").format(Math.round(ratio * 10000.0) / 100.0));}} looks curious. What is the advantage of using it instead of the more common {{String.format("%.2f", ratio * 100)}}? {{DecimalFormat}} is also used in {{VirtualHostStoreUpgraderAndRecoverer}} and {{ProducerFlowControlTest#getFlowResumeRatio}}. * In {{RingOverflowPolicyHandler}} I am curious why you use {{_queue.getEntries() instanceof PriorityQueueList}} instead of {{_queue instanceof PriorityQueue}}? * I am not sure whether the current behaviour of rejecting messages if they can't go on the RingQueue is the right one. Imagine the message limit is {{1}} and we send a message "A" of priority {{2}} concurrently with a message "B" of priority {{1}}. Now the result the application publishing "B" sees is racey (either accept or reject) while the result on the broker is identical (only message "B" is on the queue). That seems odd to me. Similar arguments apply to the other checks performed in {{RingOverflowPolicyHandler#isAcceptable}}. I'm basically arguing to kill that method. * I don't like the speciallisation for {{PriorityQueue}} in {{RingOverflowPolicyHandler#handleOverflow}}. Maybe if we had a {{Queue#getLast()}} method. A further argument in support of this is that I think the current implementation is not correct for {{SortedQueue}}. * In the Upgrader I think we should fail if we cannot upgrade something successfully instead of printing a warning and using the default. * In {{AbstractQueueTestBase}} resetting the queue to its default values at the end of the tests is not necessary because the queue is recreated before every test in the test {{setUp}} method. * The FlowControlPolicy is not tested. * Changing of attributes while messages on the queue and/or flow control enforced not tested * In 0-10, in {{ServerSession}} the check {{if (result.isRoutingFailure())}} was removed. Should it have been moved to the {{ServerSessionDelegate}} along with the initial creation of the {{routingResult}}? * In the UI I think the description of "Maximum Queue Depth (Bytes)" is ambigious. "Maximum size of messages (including header) in the queue" could mean the total size of messages on the queue or the size of each individual message on the queue. > Ring policy type > ---------------- > > Key: QPID-7618 > URL: https://issues.apache.org/jira/browse/QPID-7618 > Project: Qpid > Issue Type: New Feature > Components: Java Broker > Affects Versions: qpid-java-6.1.1, qpid-java-7.0 > Reporter: Tomas Vavricka > Labels: policy-type, queue, ring > Fix For: qpid-java-7.0 > > Attachments: 0001-QPID-7569-Ring-policy-type.patch, > overflow-policy.tar.gz > > > It would be good if Java Broker will support ring policy. > Ring policy - delete oldest message(s) when queue capacity is reached > Queue capacity can be defined by maximum count of message and maximum size of > messages (including header). -- This message was sent by Atlassian JIRA (v6.3.15#6346) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org