You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@qpid.apache.org by Andrew Kennedy <an...@gmail.com> on 2010/12/02 08:59:53 UTC
Re: Review Request: ...
Hi All,
I haven't tried this new review service yet, so I have some questions:
* How well does it work in general?
* Do you have to upload a patch or will it allow review of an already
committed change?
* How useful has it been for those of you who have used it so far?
Thanks,
Andrew.
--
-- andrew d kennedy ? do not fold, bend, spindle, or mutilate ;
-- http://grkvlt.blogspot.com/ ? edinburgh : +44 7582 293 255 ;
On 30 Nov 2010, at 20:08, Alan Conway wrote:
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/130/
> -----------------------------------------------------------
>
> Review request for qpid.
>
>
> Summary
> -------
>
> This patch is a roll up of the following 3 patches:
>
> commit 5b28517ab3890a7f5316f2c89964343b9cc0b0dd
> Author: Alan Conway <ac...@redhat.com>
> Date: Mon Nov 29 14:39:46 2010 -0500
>
> Modified cluster_tests causes broker shut down with invalid-
> argument error.
>
> Described in https://bugzilla.redhat.com/show_bug.cgi?
> id=655078. The
> management agent's deleted-object list was not being replicated
> to new
> members joining the cluster, so management generated fewer deleted
> object notifications on the newer member, causing it to fail
> with an
> invalid-argument error. The list is now being replicated
> correctly.
>
> commit 733262b4d6cec8b0d30db949bfa93f11dc07773f
> Author: Alan Conway <ac...@redhat.com>
> Date: Tue Nov 23 16:35:24 2010 -0500
>
> Add missing call to Message::setTimestamp in
> ManagementAgent::sendBufferLH.
>
> commit 32ed120146db5f756d8ea4f9a7e0330bf5716f9d
> Author: Alan Conway <ac...@redhat.com>
> Date: Tue Nov 23 15:50:00 2010 -0500
>
> Enable cluster-safe assertions on transition to CATCHUP
>
> Delaying until READY was causing multiple clientConnect management
> events to be raised, because broker::Connection::setUserId
> relies on
> sys::isCluster to avoid producing duplicate events with
>
>
> Diffs
> -----
>
> /trunk/qpid/cpp/src/cluster.mk 1040689
> /trunk/qpid/cpp/src/qpid/cluster/Cluster.cpp 1040689
> /trunk/qpid/cpp/src/qpid/cluster/Connection.h 1040689
> /trunk/qpid/cpp/src/qpid/cluster/Connection.cpp 1040689
> /trunk/qpid/cpp/src/qpid/cluster/UpdateClient.cpp 1040689
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.h 1040689
> /trunk/qpid/cpp/src/qpid/management/ManagementAgent.cpp 1040689
> /trunk/qpid/cpp/src/tests/cluster_tests.py 1040689
> /trunk/qpid/cpp/xml/cluster.xml 1040689
>
> Diff: https://reviews.apache.org/r/130/diff
>
>
> Testing
> -------
>
> Passes make check, make check-long. Currently running make check-
> long in a loop.
>
>
> Thanks,
>
> Alan
>
---------------------------------------------------------------------
Apache Qpid - AMQP Messaging Implementation
Project: http://qpid.apache.org
Use/Interact: mailto:dev-subscribe@qpid.apache.org
Re: Review Request: ...
Posted by Alan Conway <ac...@redhat.com>.
On 12/02/2010 02:59 AM, Andrew Kennedy wrote:
> Hi All,
>
> I haven't tried this new review service yet, so I have some questions:
>
> * How well does it work in general?
Works pretty well, I've used it on both sides. It lets you see the full context
of the patch you're reviewing.
> * Do you have to upload a patch or will it allow review of an already committed
> change?
So far I've only tried pre-commit reviews. Check the docs, they do talk about
post-commit reviews.
> * How useful has it been for those of you who have used it so far?
Its been useful, but if you're using git-svn you need to hack things a bit (I
would guess things work better if you just use SVN)
Attached is a script for generating a reviewboard-compatible svn-like patch from
a git branch which you can cut/paste into the RB page.
Probably a better way is to use post-review, from Andrew Stitcher:
post-review in RBTools will also do this for you (and it will also push
the review up to ReviewBoard too). However I had to patch it to get it
to work with our repo:
(I'll see if I can get the patch upstream)
--- /usr/lib/python2.7/site-packages/rbtools/postreview.py.0 2010-11-30
15:26:42.820050686 -0500
+++ /usr/lib/python2.7/site-packages/rbtools/postreview.py 2010-11-30
15:26:48.977039236 -0500
@@ -229,7 +229,7 @@
# If one of the directories doesn't match, then path is not
relative
# to root.
- if rootdirs != pathdirs:
+ if rootdirs != pathdirs[:len(rootdirs)]:
return None
# All the directories matched, so the relative path is whatever