[
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:43 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):
Review still in progress!!!
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 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.
* 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)}}?
* ...
> 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: [email protected]
For additional commands, e-mail: [email protected]
