[
https://issues.apache.org/jira/browse/QPID-3346?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13081204#comment-13081204
]
jirapos...@reviews.apache.org commented on QPID-3346:
-----------------------------------------------------
bq. On 2011-08-08 10:11:17, Gordon Sim wrote:
bq. > /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.h, line 132
bq. > <https://reviews.apache.org/r/1312/diff/1/?file=30947#file30947line132>
bq. >
bq. > I'm not keen on the terminology here. For me selector implies
something subtly different from the role this object is serving (at least from
the role I *think* it is serving).
bq. >
bq. > I'd prefer something like 'allocator'.
Me too - renamed to allocator.
bq. On 2011-08-08 10:11:17, Gordon Sim wrote:
bq. > /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.h, line 148
bq. > <https://reviews.apache.org/r/1312/diff/1/?file=30947#file30947line148>
bq. >
bq. > Again, I'm not too keen on the term 'consumed' in this context.
Though I can see how it is justified, it is potentially confusing in my view
(could imply the actual dequeue of a message).
bq. >
bq. > I'd prefer 'acquired', 'allocated' or even just 'locked' as they are
all less ambiguous on the state in question.
Agreed - renamed to acquired.
bq. On 2011-08-08 10:11:17, Gordon Sim wrote:
bq. > /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp, line 158
bq. > <https://reviews.apache.org/r/1312/diff/1/?file=30945#file30945line158>
bq. >
bq. > Not too keen on this lookup. Can it be avoided?
bq. >
bq. > E.g. can we modify the Queue::acquire() to simply take the consumer
name as the second parameter? (That is for the present at least all that is
required)
bq. >
bq. > Alternatively the DeliveryRecord could be modified to hold a pointer
to the Consumer rather than simply the tag.
Modified Queue::acquire() to take the name instead - should be ok. I
originally had added a pointer to the Consumer into the DeliveryRecord, but
that raised issues with cluster replication (Consumer not present when
replicating DeliveryRecord). May have to revisit this if it turns out we need
the pointer.
bq. On 2011-08-08 10:11:17, Gordon Sim wrote:
bq. > /branches/qpid-3346/qpid/cpp/src/qpid/broker/LegacyLVQ.cpp, line 39
bq. > <https://reviews.apache.org/r/1312/diff/1/?file=30946#file30946line39>
bq. >
bq. > The purpose of the check is to ensure that an acquire attempt for a
message that has since been replaced, does not acquire the message that has
replaced it instead.
bq. >
bq. > I believe it is still necessary, though I concede the form is ugly
and unintuitive.
Agreed that it is necessary for LVQ, but the abstract api for
'Messages::remove' can't explicitly enforce that the caller supply the actual
target msg, just the position. Given that, it's pretty easy for the caller to
get this wrong without discovering it (which actually happened to me - thank
goodness for unit tests :).
And the "move message/purge message" management operations probably could use a
remove method that doesn't necessitate a preceding message find().
I'm thinking of two different message-removal use-cases:
bool Messages::remove( position, QueuedMessage *result) - remove msg at
position, return true and set result if found
bool Messages::remove( const QueuedMessage& target ) - remove target msg,
return true if msg was found. This variant could be used by 'acquire' or
other paths that have already retrieved the QueuedMessage.
Although, defining a iterator for the Messages class probably renders this
whole issue moot.
- Kenneth
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/1312/#review1317
-----------------------------------------------------------
On 2011-08-05 20:46:15, Kenneth Giusti wrote:
bq.
bq. -----------------------------------------------------------
bq. This is an automatically generated e-mail. To reply, visit:
bq. https://reviews.apache.org/r/1312/
bq. -----------------------------------------------------------
bq.
bq. (Updated 2011-08-05 20:46:15)
bq.
bq.
bq. Review request for qpid.
bq.
bq.
bq. Summary
bq. -------
bq.
bq. Some initial refactoring of Queue/Consumer interface to allow for message
grouping support. Very preliminary.
bq.
bq.
bq. This addresses bug qpid-3346.
bq. https://issues.apache.org/jira/browse/qpid-3346
bq.
bq.
bq. Diffs
bq. -----
bq.
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/Consumer.h 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/LegacyLVQ.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.h 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/Queue.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueEvents.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueFlowLimit.h 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueFlowLimit.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueueObserver.h 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/QueuePolicy.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/SemanticState.h 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/SemanticState.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/broker/ThresholdAlerts.h 1144324
bq. /branches/qpid-3346/qpid/cpp/src/qpid/cluster/Connection.cpp 1144324
bq. /branches/qpid-3346/qpid/cpp/src/tests/QueueTest.cpp 1144324
bq.
bq. Diff: https://reviews.apache.org/r/1312/diff
bq.
bq.
bq. Testing
bq. -------
bq.
bq. minimal - passes unit tests on linux.
bq.
bq.
bq. Thanks,
bq.
bq. Kenneth
bq.
bq.
> Support message grouping with stricted sequence consumption across multiple
> consumers.
> --------------------------------------------------------------------------------------
>
> Key: QPID-3346
> URL: https://issues.apache.org/jira/browse/QPID-3346
> Project: Qpid
> Issue Type: New Feature
> Components: C++ Broker
> Affects Versions: 0.12
> Reporter: Ken Giusti
> Assignee: Ken Giusti
> Fix For: 0.14
>
> Attachments: msg_groups_0.2.txt
>
>
> This feature is described in the attached QIP as Policy #2: Sequenced
> Consumers.
--
This message is automatically generated by JIRA.
For more information on JIRA, see: http://www.atlassian.com/software/jira
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscr...@qpid.apache.org
