[jira] [Commented] (QPID-7815) Reject policy type

classic Classic list List threaded Threaded
1 message Options
Reply | Threaded
Open this post in threaded view

[jira] [Commented] (QPID-7815) Reject policy type

JIRA jira@apache.org

    [ 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 [hidden email]
** 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

To unsubscribe, e-mail: [hidden email]
For additional commands, e-mail: [hidden email]