qpid-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Alan Conway" <acon...@redhat.com>
Subject Re: Review Request: QPID-3079: allow asynchronous completion of Message.Accept command (note: requires store interface changes).
Date Thu, 09 Jun 2011 20:51:15 GMT

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/860/#review795
-----------------------------------------------------------



/branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h
<https://reviews.apache.org/r/860/#comment1713>

    Why indirection via a factory? Why not just pass callback pointer?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp
<https://reviews.apache.org/r/860/#comment1714>

    Why can't we do a map lookup here by queue name? Why the search by PersistenceId?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp
<https://reviews.apache.org/r/860/#comment1715>

    Looks like meaning of synclist has been reversed here - old code added to list on dequeue,
new code removes from list. Is that right?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h
<https://reviews.apache.org/r/860/#comment1716>

    why the indirection via a factory?
    



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h
<https://reviews.apache.org/r/860/#comment1717>

    Overloading operator () is just confusing. Give it a function name.



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp
<https://reviews.apache.org/r/860/#comment1718>

    Do we need a map here? Could we store the callback directly on the PersistentMessage?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp
<https://reviews.apache.org/r/860/#comment1719>

    This would be simpler if the callback was stored on the message rather than in a map



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/860/#comment1720>

    Do we need the uniqueness logic? How often are we likely to see duplicate queues in the
list? Flushing is an idempotent operation, so flushing twice would be an error.



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/860/#comment1721>

    Could this callback be attached to the message instead, avoiding heap allocation?



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/860/#comment1722>

    I think that using a factory to avoid construction of the callback is more expensive than
always constructing the calback, especially if the callback is made part of the Message so
there's no heap allocation.



/branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp
<https://reviews.apache.org/r/860/#comment1723>

    Possible cluster issue. The cluster replicates the unacked list. Is this change predictable
for the cluster?


- Alan


On 2011-06-08 20:31:05, Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/860/
> -----------------------------------------------------------
> 
> (Updated 2011-06-08 20:31:05)
> 
> 
> Review request for qpid, Alan Conway, Gordon Sim, and Kim van der Riet.
> 
> 
> Summary
> -------
> 
> Modifies the broker's handling of Message.Accept to hold off the completion of the command
until all messages related to the accept have completed dequeue.  This particularly applies
to persistent messages, as the store::dequeue() operation must complete before the message
is considered fully dequeued.
> 
> Note this bugfix requires some changes to the broker's store module interface:  previously,
the store only identified the message when a dequeue was completed.  This is not enough information
- the queue from which is was removed must also be identified (the message may be in the process
of being dequeued on several queues at once).
> 
> 
> This addresses bug qpid-3079.
>     https://issues.apache.org/jira/browse/qpid-3079
> 
> 
> Diffs
> -----
> 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/AsyncCompletion.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Message.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStore.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/MessageStoreModule.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/NullMessageStore.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableQueue.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/Queue.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/RecoverableQueue.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/RecoveryManagerImpl.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SemanticState.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionContext.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/SessionState.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/tests/AsyncCompletion.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/tests/QueueTest.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/tests/TestMessageStore.h 1124895 
> 
> Diff: https://reviews.apache.org/r/860/diff
> 
> 
> Testing
> -------
> 
> broker unit tests, store unit tests (modified jboss store).   Still needs to be vetted
on non-linux, and have latest trunk merged in.
> 
> 
> Thanks,
> 
> Kenneth
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message