[
https://issues.apache.org/jira/browse/QPID-7815?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16046673#comment-16046673
]
Lorenz Quack commented on QPID-7815:
------------------------------------
Hi Tomas,
Again, thanks for the contribution.
However, I do have some review comments:
* {{AccessController.getContext()}} is an expensive operation so you may want
to cache it the first time you need it in
{{RejectOverflowPolicyHandler#checkOverflow}}
* I wonder whether in the AMQP 1.0 path {{amqp:resource-limit-exceeded}} would
be more appropriate than {{amqp:precondition-failed}}. Any particular reason
you chose the latter?
* The closing due to rejection should happen on the IO thread. In the AMQP 1.0
path you do this but not in the other code paths.
* In AMQP 0-9 I think the method you were looking for is
{{AMQChannel#closeChannel}} instead of {{AMQChannel#close}}
* The {{QueuePolicyTest}} is currently only activated on 0-10. IMHO, that is
not sufficient test coverage for a new feature. I'll attach a patch enabling
the test on all protocols. It currently fails on 0-9 and 1.0. Have you tested
those protocols?
* I think we need to collectively decide what behaviour is desirable when the
queue limits change while messages reside on the queue. Should the limits be
retroactively be enforced by silently dropping messages or will the limits only
apply to new messages? We might want to take that discussion to users@q.a.o
** If we want to dynamically react to the attributes changes on the Queue your
OverflowPolicy currently does not react to that. When the next message arrives
or the Housekeeping thread kicks in it would then start dropping messages. If
you want to dynamically react to changes you would have to register yourself as
a ChangeListener on the Queue. See {{FlowToDiskOverflowPolicyHandler}} as an
example on how this can be done. Note, that there we create a member that does
the actual checking. This is done to prevent an unsafe publication of {{this}}
from the constructor.
** The above behaviour means that we can silently drop messages. This makes
this Overflow policy as dangerous as the {{RingOverflowPolicy}}.
** If we do not want to retroactively react to limit changes then we would not
need the {{Queue#getNewestEntry()}} method because we are passing the new
QueueEntry to the OverflowPolicy.
> Reject policy type
> ------------------
>
> Key: QPID-7815
> URL: https://issues.apache.org/jira/browse/QPID-7815
> Project: Qpid
> Issue Type: New Feature
> Components: Java Broker
> Affects Versions: qpid-java-broker-7.0.0
> Reporter: Tomas Vavricka
> Assignee: Lorenz Quack
> Labels: policy-type, queue, reject
> Fix For: qpid-java-broker-7.0.0
>
> Attachments:
> 0001-QPID-7815-Java-Broker-Enable-QueuePolicyTests-on-all.patch,
> 0001-QPID-7815-Reject-policy-type.patch
>
>
> It would be good if Java Broker will support reject policy.
> Reject policy - reject incoming 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.4.14#64029)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org
For additional commands, e-mail: dev-h...@qpid.apache.org
