Re: we need more reviews

[Noah Slater <ns...@apache.org>]

Done: https://issues.apache.org/jira/browse/INFRA-7254

On 23 January 2014 14:12, Noah Slater <ns...@apache.org> wrote:
> Okay, I'll leave this a few days and then request a Review Board
> instance so we can experiment with it.
>
> Will be entirely voluntary! If it starts to become useful, we can
> codify how we expect it to be used. Until that point, we can just send
> our patches to Review Board and ask for a review on the mailing list.
>
> On 23 January 2014 03:30, Mutton, James <jm...@akamai.com> wrote:
>> On 1/22/14 3:07 PM, "Russell Branca" <ch...@apache.org> wrote:
>>
>>
>>>Huge +1 to more review.
>>>
>>>Let's also setup some code guidelines for Erlang/Javascript/C/HTML so we
>>>have an authoritative list of rules to follow to ensure code consistency,
>>>and similarly, let's get some guidelines for git commits messages as well,
>>>Tim Pope's article comes to mind:
>>>http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html.
>>>
>>>Whichever review tool we decide to use, we should use it universally. We
>>>should figure out a way to do github integration so that PR's show up in
>>>the tool, and then we update the PR with a link to the relevant review
>>>page. We also need a good way to initiate the analogue of a PR in whatever
>>>tool we use. One of the complications with the review process today, is
>>>that we're all accustomed to doing review through github PR's, but we can
>>>use github PR's for branches who exist solely on the ASF origin. I imagine
>>>this won't be an issue with Hive or Gerrit, but we should verify that
>>>we're
>>>not required to initiate code review from third party origins.
>>>
>>>On a side note, one of my personal nits with github PR's, that I hope
>>>might
>>>be resolved with one of these tools, is losing comments when the code
>>>changes. For instance, if I do a review and make 10 comments that each
>>>represent a line item that needs to be addressed, I would like to see a
>>>task list on the review indicating there are unfinished items to be
>>>addressed, basically a way to make a set of checkboxes necessary to
>>>complete the review. I find it odd that github doesn't do this, even more
>>>so by the fact that changing code to address comments will often mess with
>>>the commenting and hide the messages you posted, making it challenging to
>>>decipher whether the review items have been addressed. This is by no means
>>>a requirement to using any approach, just hoping that someone knows of a
>>>good way to approach this problem in whatever tool we use.
>>>
>>>
>>>-Russell
>>>
>>>
>>>On Wed, Jan 22, 2014 at 6:06 AM, Benoit Chesneau
>>><bc...@gmail.com>wrote:
>>>
>>>> On Wed, Jan 22, 2014 at 12:47 PM, Noah Slater <ns...@apache.org>
>>>>wrote:
>>>>
>>>> > My first comment is: if we want more reviews, let's have more
>>>>committers.
>>>> >
>>>> > We double our committer base in 2013, and the results look like this:
>>>> >
>>>> >     https://www.ohloh.net/p/couchdb/analyses/latest/languages_summary
>>>> >
>>>> > And I see comments like this on StackOverflow:
>>>> >
>>>> >     "I've recently noticed that Couch DB is back in heavy
>>>>development."
>>>>
>>>>
>>>> > So I think we should continue to aggressively recruit more committers
>>>> > to the project in 2014. Excelsior!
>>>> >
>>>>
>>>> Welcoming new committers in the project is certainly a good thing but I
>>>> think it's orthogonal to the need of more review and team work. We
>>>>should
>>>> find a good workflow now while we are still a limited number of
>>>>committers
>>>> because It will be harder to enforce anything on a larger team. We
>>>>should
>>>> settle on some guidelines now. It will also help us to attract more
>>>>people
>>>> IMO.
>>>>
>>>>
>>>> >
>>>> > However, as for the way we do reviews...
>>>> >
>>>> > Infra ticket about Gerrit
>>>> > https://issues.apache.org/jira/browse/INFRA-2205
>>>> >
>>>> > Effort seems to be mothballed. But I'm sure we could restart it if we
>>>> > were serious.
>>>> >
>>>> > However, we could use this:
>>>> >
>>>> > https://reviews.apache.org/groups/hive/
>>>> >
>>>> > It is well integrated with Apache infrastructure already, sends mails
>>>> > to the mailing list, and so on.
>>>> >
>>>> > Happy to request an instance and we can experiment with it if we like.
>>>> > If it doesn't work out, we stop using it. No harm. Could make it
>>>> > entirely voluntary until we figure out a workflow that we all like.
>>>> >
>>>> > Should I do that?
>>>> >
>>>>
>>>> I thought gerrit was already integrated but any tool that could help us
>>>>to
>>>> make the review is OK for me. How hive compares to gerrit? Could we also
>>>> plug it with github like gerrit [1] ?
>>>>
>>>> - benoît
>>>>
>>>> [1] https://gerrit.googlesource.com/plugins/github/ (and some others)
>>>>
>>>> >
>>>> > On 20 January 2014 15:30, Nick North <no...@gmail.com> wrote:
>>>> > > On 20 January 2014 12:26, Dale Harvey <da...@arandomurl.com> wrote:
>>>> > >
>>>> > >> I fully agree, its something I mentioned at the couchdb conf in
>>>> > vancouver,
>>>> > >> a review first system encourages contributions and has multiple
>>>> benefits
>>>> > >>
>>>> > >>  * At least 2 people look at the code, less likely to push silly
>>>> > mistakes
>>>> > >>  * Can codify and practice review rules
>>>> > >>  * Its much easier to view the current activity in the project
>>>> > >>  * Can bring up points of discussion before its too late
>>>> > >>
>>>> > >> But I think the most important thing is that it removes the burden
>>>>of
>>>> > >> responsibility from the committer to the project as a whole, also
>>>> hugely
>>>> > >> beneficial is that it makes it particularly obvious when a patch
>>>>has
>>>> > reach
>>>> > >> a stalemate and forces someone to make the call.
>>>> > >>
>>>> > >> For reference on PouchDB every committer is trusted to push code,
>>>> nobody
>>>> > >> (including myself) pushes their own code to master, it goes in the
>>>>PR
>>>> > queue
>>>> > >> and gets a +1 from any other committer (who will usually push it),
>>>> thats
>>>> > >> essentially the same process we use at mozilla and coming to think
>>>>of
>>>> it
>>>> > >> any other project I have worked on, any commiter has the ability to
>>>> -1 a
>>>> > >> patch at which point they give a reason and usually some solution
>>>>gets
>>>> > >> agreed on
>>>> > >>
>>>> > >
>>>> > > I like this system, with one small quibble about responsibility. I
>>>> don't
>>>> > > think it should be seen as removing the burden of responsibility
>>>>from
>>>> the
>>>> > > committer to the project as a whole. It becomes easy then for
>>>>everyone,
>>>> > > including the committer, to think someone else will find bugs, and
>>>> no-one
>>>> > > puts in enough effort. I'd say it is still primarily the
>>>>responsibility
>>>> > of
>>>> > > the committer to ensure that code is error free, but that at least
>>>>one
>>>> > > person who knows that area of the code base should sign off on it.
>>>>Some
>>>> > > spreading of responsibility is good, but too much can actually lead
>>>>to
>>>> a
>>>> > > decline in quality.
>>>> > >
>>>> > >
>>>> > > Nick
>>>> >
>>>> >
>>>> >
>>>> > --
>>>> > Noah Slater
>>>> > https://twitter.com/nslater
>>>> >
>>>>
>>
>>
>> +1 to reviews and getting code guidelines.  Nice to see that Apache has a
>> reviewboard instance available (https://reviews.apache.org/groups/ hive
>> link).  I've spent a lot of time using reviewboard, it's got a good UI and
>> API.  Reviews are submitted using a separate command line tool
>> (post-review) to actually post the review, but it lets you make multiple
>> updates from feedback and allows others to see the diffs on the same
>> review so you can see how a change evolves.  You can leave comments or put
>> a "ship-it" tag to +1 the commit, then from a distance it's possible to
>> see all the reviews outstanding and how many votes, if any, they've
>> received.  It's friendly for non-blocking reviews too, since the code
>> review process is separate.  It also lets you can work on reviews a little
>> at a time and save them in between instead of having to devote a block of
>> time to code reviews (I do this sometimes on big changes).
>>
>> Garrit wants to gate the source-tree if I remember correctly so is
>> possibly less friendly with remote repositories.
>>
>> </JamesM>
>>
>
>
>
> --
> Noah Slater
> https://twitter.com/nslater



-- 
Noah Slater
https://twitter.com/nslater