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 Fri, 17 Jun 2011 13:19:13 GMT

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


Looking good, just a few suggestions.

Note on testing: need to run store tests as well as qpidd tests.


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

    Why the change from dequeue to list?



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

    Static locals not thread safe, use a normal local variable.



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

    



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

    What happens in the case isRedundant() == true?



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

    Functions are too big to inline in .h, move to .cpp.



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

    Suggest a change here to make it more flexible and type safe:
    
    registerCallback(boost::function<void()> f);
    
    You can package up all the details in boost::bind at the call site, this class doesn't
need to know. See comments below



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

    Store the DequeueCompletion pointer on the PersistableMessage, avoid this map lookup.



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

    static local variables are not thread safe. Use a normal local variable constructing an
intrusive pointer is trivial.



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

    This doesn't need to be static, and you can avoid the casting. Just write
    
    void dequeueDone() { cmd->complete(); }
    
    and see further comment at the call site below



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

    See comments above: this can now be
    
    async->registerCallback(boost::bind(&AsyncMessageAcceptCmd::dequeueDone, this))


- Alan


On 2011-06-16 15:25:17, Kenneth Giusti wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/860/
> -----------------------------------------------------------
> 
> (Updated 2011-06-16 15:25:17)
> 
> 
> 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/DeliveryRecord.h 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/DeliveryRecord.cpp 1124895 
>   /branches/qpid-3079/qpid/cpp/src/qpid/broker/PersistableMessage.cpp 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/SemanticState.cpp 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