[
https://issues.apache.org/jira/browse/QPID-7618?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15826355#comment-15826355
]
Keith Wall edited comment on QPID-7618 at 1/17/17 5:54 PM:
-----------------------------------------------------------
On 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
was (Author: k-wall):
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
