[
https://issues.apache.org/jira/browse/QPID-7618?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15880326#comment-15880326
]
Lorenz Quack commented on QPID-7618:
------------------------------------
Some more review comments of the new patch set:
* {{ProducerFlowControlOverflowPolicyHandler.Handler#isUndefull}} still has the
typo with the missing "r"
* {{ProducerFlowControlOverflowPolicyHandler.Handler}} and
{{ProducerFlowControlOverflowPolicyHandler.ProducerFlowControlListener}}
*could* be merged into one class
* In {{ProducerFlowControlOverflowPolicyHandler.Handler#checkOverflow(long,
long)}} I prefer explicit parenthesis around the logical boolean operations in
the if-statement to make the precedence between {{&&}} and {{||}} explicit. I
can never remember the implicit rules.
* The {{ProducerFlowControlOverflowPolicyHandler.Handler}} logic is not
thread-safe. I try to describe one unlucky case. One thread calls
{{checkOverflow()}}. While it is in the {{checkUnderflow}} but before it reset
the {{_overfull}} flag the thread is interrupted by a second thread that also
calls {{checkOverflow()}}. After the second thread checks the {{_overfull}}
flag but before it blocks the session the first thread resumes, resets the flag
and unblocks the sessions. When the second thread resumes it adds its session
and proceeds to {{checkUnderflow}} where it immediately bails out because the
{{_overfull}} flag has been reset by the first thread. This means the second
thread will not unblock its session even though it should. This session will
only be unblocked if the Queue get blocked and unblocked again.
* The names used in {{ProducerFlowControlOverflowPolicyHandler}} are a bit
inconsistent. Sometimes we use {{overfull}} sometimes {{overflow}}. Same for
{{under*}}. The operational logmessages go with {{overfull}} and {{underfull}}.
* I think you misunderstood my comment about factoring out a method. You
factored out {{checkOverflow}} while I was talking about factoring out
{{checkUnderflow}}. So that you can avoid the calls to {{isUndefull}} when
called from the {{ProducerFlowControlListener}}.
* The debug log message in {{AbstractQueue#deleteEntry}} is no longer correct
since this can now also be called from the RingQueuePolicy.
* The order of the arguments in the call to {{QueueMessages.DROPPED}} do not
line up with the parameters.
* The arguments 2 and 3 of {{QueueMessages.OVERFULL}} and
{{QueueMessages.UNDERFULL}} are not used. instead you accidentally reuse 0 and
1.
* Why is the complication with checking the keys in
{{SortedQueueEntryList#getLastEntry}} needed? should it not be sufficient to
just iterate to the end of the list? Furthermore, I just discovered
{{QueueEntryList#getTail}}. Is this not exactly what we are trying to do with
{{QueueEntryList#getLastEntry}}?
* You misunderstood my comment about the UI. I was saying that the {{prompt}}
and {{title}} field contain ambiguous statements. From those statements it is
not clear whether the maximum applies to each individual message or to the
queue as a whole (the sum of messages). I think the label was fine the way it
was.
* Some of the DecimalFormat I mentioned above are still in the code.
Some further comments not specific to the latest patch:
* Why was the validation in {{AbstractQueue#validateChange}} removed?
* I think the newly introduced {{AmqpError#valueOf(java.lang.String)}} is
unused. It is a bit questionable in the first place since
{{AmqpError.valueOf("internal-error")}} would succeed but
{{AmqpError.valueOf((Object)"internal-error")}} would fail. checking {{obj
instance of String}} in the existing {{AmqpError#valueOf(java.lang.Object)}}
would have probably been better.
> 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
