[ https://issues.apache.org/jira/browse/QPID-7618?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15826355#comment-15826355 ]
Keith Wall commented on QPID-7618: ---------------------------------- One the whole this looks good. I have a few comments: h3. Comments on the Model We now have both producer flow control and ring queues. Both features constrain a queue's size, but are currently configured by different attributes. Ring uses Queue#maxSize/#maxCount and producer flow control uses Queue#queueFlowControlSizeBytes and Queue#queueFlowResumeSizeBytes. As I user, how would I know which attribute applies to which or how would I know what to expect if I configured both features? I wonder if we should re-model producer flow control as a Policy. * Introduce a third policy type {{PolicyType.FLOW_CONTROL}}. * Change the producer flow control algorithm to respect maxSize/maxCount. * {{queueFlowResumeSizeBytes}} has always seemed rather low level and probably not interesting to most users. I think {{queueFlowResumeSizeBytes}} should be replaced with a context variable provides a resume point as a percentage which perhaps defaults to 50%. Users would be able to override if need be. * For existing users using producer flow control an automated configuration converter would handle the upgrade of the queue configuration. What do you think? I realise that you may not have knowledge of the producer flow control feature/configuration upgraders, so we may need to help with this part of the change. h3. Comments on the code: # Class name PolicyType is rather non-specific. We already have other policy such as {{LifetimePolicy}} and {{ExclusivityPolicy}}. I'd suggest {{OverflowPolicy}} for the new one. No change suggested for the wire arguments as I see you are aiming for compatibility with the C++ Broker at wire level (which I think it the right approach). # What was the motivation for calling enforcing the policy OrderedQueueEntryList.java:90 within the {{add()}}? For a LVQ that is full, this could mean that the oldest queue entry is discarded unnecessarily. I wonder if the responsibility for enforcement of the policy should be AbstractQueue#doEnqueue. This would keep enforcement of the policy a private concern to AbstractQueue which I think would be preferable. # Queues (except SortedQueues) are lock free by design. Multiple connections may be enqueuing to a single queue at once. I think the code that enforces maxCount (AbstractQueueEntryList#applyPolicyType#66) assumes a single threaded model. Unlucky thread scheduling of two publishing connection threads could allow count to be breached. I think the ‘if’ needs to be turned into a ‘while.’ This does mean that faced with racing threads too many queue entries be dequeued. I don’t see a easy way around this and don’t think this would present a problem in practice (maxSize enforcement has this feature). # {{AttributeValueConverter#EnumConverter}} - why is capitalising the value important? The domain of values for an attribute used within the model don't necessarily need to follow the domain of argument on the wire. Indeed, as we are support many different protocols there are already many cases where there is divergence. I think any 'cooking' belongs in the QueueArgumentsConverter. # StandardReceivingLink_1_0.java:319 we probably ought to use the delivery outcome Rejected (1) if the link supports that outcome rather than unconditionally closing the link. I’ll get some more details. (1) http://docs.oasis-open.org/amqp/core/v1.0/os/amqp-core-messaging-v1.0-os.html#type-rejected > 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 > Attachments: 0001-QPID-7569-Ring-policy-type.patch > > > 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.4#6332) --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@qpid.apache.org For additional commands, e-mail: dev-h...@qpid.apache.org