suggestion for smoother code review process

[Ted Yu <yu...@gmail.com>]

Hi,
We have been using review board for a while to conduct code review.
One aspect I don't like the integration is that every round of review would
result in the summary and list of files (both of which could be long) to be
reposted to JIRA.
For a large project, such as HBASE-2856 or HBASE-3777, it is impossible
(without exaggeration) for a developer who didn't closely follow the
development to understand what was going on.

I want to share what I have been doing recently (by not commenting on review
board, if possible):
I would quote the snippet of code in the patch and make my comment

I think the person asking for review can post the url for review board
request on the JIRA. By not filling Bugs field, we don't incur extra
housekeeping that I mentioned earlier.
If the Groups and People fields are filled properly, there is no risk of
losing review request. In the worst case, one sentence on the JIRA can
remind related people to look at the patch again.

Note the above is just personally advice. Please don't interpret it as rule
or anything like that.

Cheers

Re: suggestion for smoother code review process

[Ted Yu <yu...@gmail.com>]

That's why I would get nervous if I step away from computer for an hour :-)

On Thu, Oct 20, 2011 at 3:06 PM, Andrew Purtell <ap...@apache.org> wrote:

> +1
>
> dev@ is not really possible to follow anymore, unless full time on HBase.
>
>
> Best regards,
>
>
>       - Andy
>
>
>
>
> >________________________________
> >From: Ted Yu <yu...@gmail.com>
> >To: dev@hbase.apache.org
> >Sent: Thursday, October 20, 2011 3:03 PM
> >Subject: suggestion for smoother code review process
> >
> >Hi,
> >We have been using review board for a while to conduct code review.
> >One aspect I don't like the integration is that every round of review
> would
> >result in the summary and list of files (both of which could be long) to
> be
> >reposted to JIRA.
> >For a large project, such as HBASE-2856 or HBASE-3777, it is impossible
> >(without exaggeration) for a developer who didn't closely follow the
> >development to understand what was going on.
> >
> >I want to share what I have been doing recently (by not commenting on
> review
> >board, if possible):
> >I would quote the snippet of code in the patch and make my comment
> >
> >I think the person asking for review can post the url for review board
> >request on the JIRA. By not filling Bugs field, we don't incur extra
> >housekeeping that I mentioned earlier.
> >If the Groups and People fields are filled properly, there is no risk of
> >losing review request. In the worst case, one sentence on the JIRA can
> >remind related people to look at the patch again.
> >
> >Note the above is just personally advice. Please don't interpret it as
> rule
> >or anything like that.
> >
> >Cheers
> >
> >
> >
>

Re: suggestion for smoother code review process

[Andrew Purtell <ap...@apache.org>]

+1

dev@ is not really possible to follow anymore, unless full time on HBase.


Best regards,


      - Andy




>________________________________
>From: Ted Yu <yu...@gmail.com>
>To: dev@hbase.apache.org
>Sent: Thursday, October 20, 2011 3:03 PM
>Subject: suggestion for smoother code review process
>
>Hi,
>We have been using review board for a while to conduct code review.
>One aspect I don't like the integration is that every round of review would
>result in the summary and list of files (both of which could be long) to be
>reposted to JIRA.
>For a large project, such as HBASE-2856 or HBASE-3777, it is impossible
>(without exaggeration) for a developer who didn't closely follow the
>development to understand what was going on.
>
>I want to share what I have been doing recently (by not commenting on review
>board, if possible):
>I would quote the snippet of code in the patch and make my comment
>
>I think the person asking for review can post the url for review board
>request on the JIRA. By not filling Bugs field, we don't incur extra
>housekeeping that I mentioned earlier.
>If the Groups and People fields are filled properly, there is no risk of
>losing review request. In the worst case, one sentence on the JIRA can
>remind related people to look at the patch again.
>
>Note the above is just personally advice. Please don't interpret it as rule
>or anything like that.
>
>Cheers
>
>
>

Re: suggestion for smoother code review process

[Akash Ashok <th...@gmail.com>]

On Fri, Oct 21, 2011 at 3:35 AM, Todd Lipcon <to...@cloudera.com> wrote:

> Hey Ted,
>
> I agree the formatting of the reviewboard comments back onto JIRA
> could be improved. I wrote the original script that does it - it's
> some nasty procmail and python.
>
Hey Todd, I would like to work on this. Also Is it nasty procmail or nasty
python ?
I could make do with nasty python, but I have absolutly no idea about
procmail :)


>
> It sounds like the FB folks are working on getting phabricator up -
> maybe it will have better JIRA integration?
>
> Let me know if you have some time to spend on improving the
> python/procmail setup with RB. I can connect you with the right infra
> people to make the change.
>
> -Todd
>
> On Thu, Oct 20, 2011 at 3:03 PM, Ted Yu <yu...@gmail.com> wrote:
> > Hi,
> > We have been using review board for a while to conduct code review.
> > One aspect I don't like the integration is that every round of review
> would
> > result in the summary and list of files (both of which could be long) to
> be
> > reposted to JIRA.
> > For a large project, such as HBASE-2856 or HBASE-3777, it is impossible
> > (without exaggeration) for a developer who didn't closely follow the
> > development to understand what was going on.
> >
> > I want to share what I have been doing recently (by not commenting on
> review
> > board, if possible):
> > I would quote the snippet of code in the patch and make my comment
> >
> > I think the person asking for review can post the url for review board
> > request on the JIRA. By not filling Bugs field, we don't incur extra
> > housekeeping that I mentioned earlier.
> > If the Groups and People fields are filled properly, there is no risk of
> > losing review request. In the worst case, one sentence on the JIRA can
> > remind related people to look at the patch again.
> >
> > Note the above is just personally advice. Please don't interpret it as
> rule
> > or anything like that.
> >
> > Cheers
> >
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

Re: suggestion for smoother code review process

[Ted Yu <yu...@gmail.com>]

Another reason of commenting in JIRA directly, for reviewing large projects,
is that the reviewer may not have ample time (3 hours or more) to write
thorough review using review board.
Before review board integration redundancy is minimized, it seems impolite
to sprinkle JIRA with multiple reviews from review board where file list,
etc become dominant.

Cheers

On Thu, Oct 20, 2011 at 4:11 PM, Nicolas Spiegelberg <ns...@fb.com>wrote:

> Step 1 for Phabricator is to reach parity with the current Review Board
> utilities.  Step 2 is to improve formatting and minimize redundancy.  I
> agree with Jonathan's comments: if you see something got added to RB/Phab,
> interact with the dialog there instead of trying to use JIRA directly.
>
> On 10/20/11 3:22 PM, "Ted Yu" <yu...@gmail.com> wrote:
>
> >I looked at John Sichi's comment, obviously issued from phabricator, for
> >HBASE-4532 @ 18/Oct/11 23:41
> >
> >I don't see much difference from feedback from review board - AFFECTED
> >FILES
> >were included.
> >
> >One thing I do like the postback from review board is the nice layout
> >viewable in Yahoo email but not gmail (strangely).
> >
> >On Thu, Oct 20, 2011 at 3:11 PM, Ted Yu <yu...@gmail.com> wrote:
> >
> >> I think the easiest improvement is to strip the list of files from
> >> reviewboard feedback.
> >>
> >> I would wait for a while to see if any volunteer comes up for the above
> >> task :-)
> >>
> >> I am not sure about phabricator which requires an account.
> >> I remember seeing phabricator feedback in JIRA. The format is different.
> >>
> >> Cheers
> >>
> >>
> >> On Thu, Oct 20, 2011 at 3:05 PM, Todd Lipcon <to...@cloudera.com> wrote:
> >>
> >>> Hey Ted,
> >>>
> >>> I agree the formatting of the reviewboard comments back onto JIRA
> >>> could be improved. I wrote the original script that does it - it's
> >>> some nasty procmail and python.
> >>>
> >>> It sounds like the FB folks are working on getting phabricator up -
> >>> maybe it will have better JIRA integration?
> >>>
> >>> Let me know if you have some time to spend on improving the
> >>> python/procmail setup with RB. I can connect you with the right infra
> >>> people to make the change.
> >>>
> >>> -Todd
> >>>
> >>> On Thu, Oct 20, 2011 at 3:03 PM, Ted Yu <yu...@gmail.com> wrote:
> >>> > Hi,
> >>> > We have been using review board for a while to conduct code review.
> >>> > One aspect I don't like the integration is that every round of review
> >>> would
> >>> > result in the summary and list of files (both of which could be
> >>>long) to
> >>> be
> >>> > reposted to JIRA.
> >>> > For a large project, such as HBASE-2856 or HBASE-3777, it is
> >>>impossible
> >>> > (without exaggeration) for a developer who didn't closely follow the
> >>> > development to understand what was going on.
> >>> >
> >>> > I want to share what I have been doing recently (by not commenting on
> >>> review
> >>> > board, if possible):
> >>> > I would quote the snippet of code in the patch and make my comment
> >>> >
> >>> > I think the person asking for review can post the url for review
> >>>board
> >>> > request on the JIRA. By not filling Bugs field, we don't incur extra
> >>> > housekeeping that I mentioned earlier.
> >>> > If the Groups and People fields are filled properly, there is no
> >>>risk of
> >>> > losing review request. In the worst case, one sentence on the JIRA
> >>>can
> >>> > remind related people to look at the patch again.
> >>> >
> >>> > Note the above is just personally advice. Please don't interpret it
> >>>as
> >>> rule
> >>> > or anything like that.
> >>> >
> >>> > Cheers
> >>> >
> >>>
> >>>
> >>>
> >>> --
> >>> Todd Lipcon
> >>> Software Engineer, Cloudera
> >>>
> >>
> >>
>
>

Re: suggestion for smoother code review process

[Jonathan Hsieh <jo...@cloudera.com>]

My tendency has always been to look first for reviews on reviewboard and to
take advantage of the interface there.  I've found the bugs file link to be
really helpful, but agree that the ugly mail to the jira is somewhat
annoying.

Jon.

On Thu, Oct 20, 2011 at 3:22 PM, Ted Yu <yu...@gmail.com> wrote:

> I looked at John Sichi's comment, obviously issued from phabricator, for
> HBASE-4532 @ 18/Oct/11 23:41
>
> I don't see much difference from feedback from review board - AFFECTED
> FILES
> were included.
>
> One thing I do like the postback from review board is the nice layout
> viewable in Yahoo email but not gmail (strangely).
>
> On Thu, Oct 20, 2011 at 3:11 PM, Ted Yu <yu...@gmail.com> wrote:
>
> > I think the easiest improvement is to strip the list of files from
> > reviewboard feedback.
> >
> > I would wait for a while to see if any volunteer comes up for the above
> > task :-)
> >
> > I am not sure about phabricator which requires an account.
> > I remember seeing phabricator feedback in JIRA. The format is different.
> >
> > Cheers
> >
> >
> > On Thu, Oct 20, 2011 at 3:05 PM, Todd Lipcon <to...@cloudera.com> wrote:
> >
> >> Hey Ted,
> >>
> >> I agree the formatting of the reviewboard comments back onto JIRA
> >> could be improved. I wrote the original script that does it - it's
> >> some nasty procmail and python.
> >>
> >> It sounds like the FB folks are working on getting phabricator up -
> >> maybe it will have better JIRA integration?
> >>
> >> Let me know if you have some time to spend on improving the
> >> python/procmail setup with RB. I can connect you with the right infra
> >> people to make the change.
> >>
> >> -Todd
> >>
> >> On Thu, Oct 20, 2011 at 3:03 PM, Ted Yu <yu...@gmail.com> wrote:
> >> > Hi,
> >> > We have been using review board for a while to conduct code review.
> >> > One aspect I don't like the integration is that every round of review
> >> would
> >> > result in the summary and list of files (both of which could be long)
> to
> >> be
> >> > reposted to JIRA.
> >> > For a large project, such as HBASE-2856 or HBASE-3777, it is
> impossible
> >> > (without exaggeration) for a developer who didn't closely follow the
> >> > development to understand what was going on.
> >> >
> >> > I want to share what I have been doing recently (by not commenting on
> >> review
> >> > board, if possible):
> >> > I would quote the snippet of code in the patch and make my comment
> >> >
> >> > I think the person asking for review can post the url for review board
> >> > request on the JIRA. By not filling Bugs field, we don't incur extra
> >> > housekeeping that I mentioned earlier.
> >> > If the Groups and People fields are filled properly, there is no risk
> of
> >> > losing review request. In the worst case, one sentence on the JIRA can
> >> > remind related people to look at the patch again.
> >> >
> >> > Note the above is just personally advice. Please don't interpret it as
> >> rule
> >> > or anything like that.
> >> >
> >> > Cheers
> >> >
> >>
> >>
> >>
> >> --
> >> Todd Lipcon
> >> Software Engineer, Cloudera
> >>
> >
> >
>



-- 
// Jonathan Hsieh (shay)
// Software Engineer, Cloudera
// jon@cloudera.com

Re: suggestion for smoother code review process

[Nicolas Spiegelberg <ns...@fb.com>]

Step 1 for Phabricator is to reach parity with the current Review Board
utilities.  Step 2 is to improve formatting and minimize redundancy.  I
agree with Jonathan's comments: if you see something got added to RB/Phab,
interact with the dialog there instead of trying to use JIRA directly.

On 10/20/11 3:22 PM, "Ted Yu" <yu...@gmail.com> wrote:

>I looked at John Sichi's comment, obviously issued from phabricator, for
>HBASE-4532 @ 18/Oct/11 23:41
>
>I don't see much difference from feedback from review board - AFFECTED
>FILES
>were included.
>
>One thing I do like the postback from review board is the nice layout
>viewable in Yahoo email but not gmail (strangely).
>
>On Thu, Oct 20, 2011 at 3:11 PM, Ted Yu <yu...@gmail.com> wrote:
>
>> I think the easiest improvement is to strip the list of files from
>> reviewboard feedback.
>>
>> I would wait for a while to see if any volunteer comes up for the above
>> task :-)
>>
>> I am not sure about phabricator which requires an account.
>> I remember seeing phabricator feedback in JIRA. The format is different.
>>
>> Cheers
>>
>>
>> On Thu, Oct 20, 2011 at 3:05 PM, Todd Lipcon <to...@cloudera.com> wrote:
>>
>>> Hey Ted,
>>>
>>> I agree the formatting of the reviewboard comments back onto JIRA
>>> could be improved. I wrote the original script that does it - it's
>>> some nasty procmail and python.
>>>
>>> It sounds like the FB folks are working on getting phabricator up -
>>> maybe it will have better JIRA integration?
>>>
>>> Let me know if you have some time to spend on improving the
>>> python/procmail setup with RB. I can connect you with the right infra
>>> people to make the change.
>>>
>>> -Todd
>>>
>>> On Thu, Oct 20, 2011 at 3:03 PM, Ted Yu <yu...@gmail.com> wrote:
>>> > Hi,
>>> > We have been using review board for a while to conduct code review.
>>> > One aspect I don't like the integration is that every round of review
>>> would
>>> > result in the summary and list of files (both of which could be
>>>long) to
>>> be
>>> > reposted to JIRA.
>>> > For a large project, such as HBASE-2856 or HBASE-3777, it is
>>>impossible
>>> > (without exaggeration) for a developer who didn't closely follow the
>>> > development to understand what was going on.
>>> >
>>> > I want to share what I have been doing recently (by not commenting on
>>> review
>>> > board, if possible):
>>> > I would quote the snippet of code in the patch and make my comment
>>> >
>>> > I think the person asking for review can post the url for review
>>>board
>>> > request on the JIRA. By not filling Bugs field, we don't incur extra
>>> > housekeeping that I mentioned earlier.
>>> > If the Groups and People fields are filled properly, there is no
>>>risk of
>>> > losing review request. In the worst case, one sentence on the JIRA
>>>can
>>> > remind related people to look at the patch again.
>>> >
>>> > Note the above is just personally advice. Please don't interpret it
>>>as
>>> rule
>>> > or anything like that.
>>> >
>>> > Cheers
>>> >
>>>
>>>
>>>
>>> --
>>> Todd Lipcon
>>> Software Engineer, Cloudera
>>>
>>
>>

Re: suggestion for smoother code review process

[Ted Yu <yu...@gmail.com>]

I looked at John Sichi's comment, obviously issued from phabricator, for
HBASE-4532 @ 18/Oct/11 23:41

I don't see much difference from feedback from review board - AFFECTED FILES
were included.

One thing I do like the postback from review board is the nice layout
viewable in Yahoo email but not gmail (strangely).

On Thu, Oct 20, 2011 at 3:11 PM, Ted Yu <yu...@gmail.com> wrote:

> I think the easiest improvement is to strip the list of files from
> reviewboard feedback.
>
> I would wait for a while to see if any volunteer comes up for the above
> task :-)
>
> I am not sure about phabricator which requires an account.
> I remember seeing phabricator feedback in JIRA. The format is different.
>
> Cheers
>
>
> On Thu, Oct 20, 2011 at 3:05 PM, Todd Lipcon <to...@cloudera.com> wrote:
>
>> Hey Ted,
>>
>> I agree the formatting of the reviewboard comments back onto JIRA
>> could be improved. I wrote the original script that does it - it's
>> some nasty procmail and python.
>>
>> It sounds like the FB folks are working on getting phabricator up -
>> maybe it will have better JIRA integration?
>>
>> Let me know if you have some time to spend on improving the
>> python/procmail setup with RB. I can connect you with the right infra
>> people to make the change.
>>
>> -Todd
>>
>> On Thu, Oct 20, 2011 at 3:03 PM, Ted Yu <yu...@gmail.com> wrote:
>> > Hi,
>> > We have been using review board for a while to conduct code review.
>> > One aspect I don't like the integration is that every round of review
>> would
>> > result in the summary and list of files (both of which could be long) to
>> be
>> > reposted to JIRA.
>> > For a large project, such as HBASE-2856 or HBASE-3777, it is impossible
>> > (without exaggeration) for a developer who didn't closely follow the
>> > development to understand what was going on.
>> >
>> > I want to share what I have been doing recently (by not commenting on
>> review
>> > board, if possible):
>> > I would quote the snippet of code in the patch and make my comment
>> >
>> > I think the person asking for review can post the url for review board
>> > request on the JIRA. By not filling Bugs field, we don't incur extra
>> > housekeeping that I mentioned earlier.
>> > If the Groups and People fields are filled properly, there is no risk of
>> > losing review request. In the worst case, one sentence on the JIRA can
>> > remind related people to look at the patch again.
>> >
>> > Note the above is just personally advice. Please don't interpret it as
>> rule
>> > or anything like that.
>> >
>> > Cheers
>> >
>>
>>
>>
>> --
>> Todd Lipcon
>> Software Engineer, Cloudera
>>
>
>

Re: suggestion for smoother code review process

[Ted Yu <yu...@gmail.com>]

I think the easiest improvement is to strip the list of files from
reviewboard feedback.

I would wait for a while to see if any volunteer comes up for the above task
:-)

I am not sure about phabricator which requires an account.
I remember seeing phabricator feedback in JIRA. The format is different.

Cheers

On Thu, Oct 20, 2011 at 3:05 PM, Todd Lipcon <to...@cloudera.com> wrote:

> Hey Ted,
>
> I agree the formatting of the reviewboard comments back onto JIRA
> could be improved. I wrote the original script that does it - it's
> some nasty procmail and python.
>
> It sounds like the FB folks are working on getting phabricator up -
> maybe it will have better JIRA integration?
>
> Let me know if you have some time to spend on improving the
> python/procmail setup with RB. I can connect you with the right infra
> people to make the change.
>
> -Todd
>
> On Thu, Oct 20, 2011 at 3:03 PM, Ted Yu <yu...@gmail.com> wrote:
> > Hi,
> > We have been using review board for a while to conduct code review.
> > One aspect I don't like the integration is that every round of review
> would
> > result in the summary and list of files (both of which could be long) to
> be
> > reposted to JIRA.
> > For a large project, such as HBASE-2856 or HBASE-3777, it is impossible
> > (without exaggeration) for a developer who didn't closely follow the
> > development to understand what was going on.
> >
> > I want to share what I have been doing recently (by not commenting on
> review
> > board, if possible):
> > I would quote the snippet of code in the patch and make my comment
> >
> > I think the person asking for review can post the url for review board
> > request on the JIRA. By not filling Bugs field, we don't incur extra
> > housekeeping that I mentioned earlier.
> > If the Groups and People fields are filled properly, there is no risk of
> > losing review request. In the worst case, one sentence on the JIRA can
> > remind related people to look at the patch again.
> >
> > Note the above is just personally advice. Please don't interpret it as
> rule
> > or anything like that.
> >
> > Cheers
> >
>
>
>
> --
> Todd Lipcon
> Software Engineer, Cloudera
>

Re: suggestion for smoother code review process

[Todd Lipcon <to...@cloudera.com>]

Hey Ted,

I agree the formatting of the reviewboard comments back onto JIRA
could be improved. I wrote the original script that does it - it's
some nasty procmail and python.

It sounds like the FB folks are working on getting phabricator up -
maybe it will have better JIRA integration?

Let me know if you have some time to spend on improving the
python/procmail setup with RB. I can connect you with the right infra
people to make the change.

-Todd

On Thu, Oct 20, 2011 at 3:03 PM, Ted Yu <yu...@gmail.com> wrote:
> Hi,
> We have been using review board for a while to conduct code review.
> One aspect I don't like the integration is that every round of review would
> result in the summary and list of files (both of which could be long) to be
> reposted to JIRA.
> For a large project, such as HBASE-2856 or HBASE-3777, it is impossible
> (without exaggeration) for a developer who didn't closely follow the
> development to understand what was going on.
>
> I want to share what I have been doing recently (by not commenting on review
> board, if possible):
> I would quote the snippet of code in the patch and make my comment
>
> I think the person asking for review can post the url for review board
> request on the JIRA. By not filling Bugs field, we don't incur extra
> housekeeping that I mentioned earlier.
> If the Groups and People fields are filled properly, there is no risk of
> losing review request. In the worst case, one sentence on the JIRA can
> remind related people to look at the patch again.
>
> Note the above is just personally advice. Please don't interpret it as rule
> or anything like that.
>
> Cheers
>



-- 
Todd Lipcon
Software Engineer, Cloudera