[
https://issues.apache.org/jira/browse/QPID-7618?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15876286#comment-15876286
]
Lorenz Quack commented on QPID-7618:
------------------------------------
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}}
* 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?
* 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.
* 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("%f.2", 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: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
