Mailing-List: contact dev-help@couchdb.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@couchdb.apache.org
MIME-Version: 1.0
In-Reply-To: 
 <CAPaJBx5mQ7rVE0h--oRBRuqK4pQvAapv=sSRScuLWCOvqQStmQ@mail.gmail.com>
References: 
 <CAFUSCh8fk98-eLEYHVws+Kk6B4sB43mkrqLYS4TTMBPC2t-ofw@mail.gmail.com>
	<CF05BAA7.143B70%jmutton@akamai.com>
	<CAPaJBx5mQ7rVE0h--oRBRuqK4pQvAapv=sSRScuLWCOvqQStmQ@mail.gmail.com>
Date: Sat, 1 Feb 2014 15:31:23 +0100
Message-ID: 
 <CAPaJBx4p2Yfyuf0u0uSwQTAt7RAewV_fLJX0bge3mARAm60v4Q@mail.gmail.com>
Subject: Re: we need more reviews
From: Noah Slater <nslater@apache.org>
To: Noah Slater <nslater@apache.org>
Cc: "dev@couchdb.apache.org" <dev@couchdb.apache.org>
Content-Type: text/plain; charset=ISO-8859-1
Content-Transfer-Encoding: quoted-printable

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

On 23 January 2014 14:12, Noah Slater <nslater@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 <jmutton@akamai.com> wrote:
>> On 1/22/14 3:07 PM, "Russell Branca" <chewbranca@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 wel=
l,
>>>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 whatev=
er
>>>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 ca=
n
>>>use github PR's for branches who exist solely on the ASF origin. I imagi=
ne
>>>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 mor=
e
>>>so by the fact that changing code to address comments will often mess wi=
th
>>>the commenting and hide the messages you posted, making it challenging t=
o
>>>decipher whether the review items have been addressed. This is by no mea=
ns
>>>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
>>><bchesneau@gmail.com>wrote:
>>>
>>>> On Wed, Jan 22, 2014 at 12:47 PM, Noah Slater <nslater@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_summar=
y
>>>> >
>>>> > 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 committer=
s
>>>> > 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 w=
e
>>>> > were serious.
>>>> >
>>>> > However, we could use this:
>>>> >
>>>> > https://reviews.apache.org/groups/hive/
>>>> >
>>>> > It is well integrated with Apache infrastructure already, sends mail=
s
>>>> > to the mailing list, and so on.
>>>> >
>>>> > Happy to request an instance and we can experiment with it if we lik=
e.
>>>> > 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 u=
s
>>>>to
>>>> make the review is OK for me. How hive compares to gerrit? Could we al=
so
>>>> plug it with github like gerrit [1] ?
>>>>
>>>> - beno=EEt
>>>>
>>>> [1] https://gerrit.googlesource.com/plugins/github/ (and some others)
>>>>
>>>> >
>>>> > On 20 January 2014 15:30, Nick North <north.n@gmail.com> wrote:
>>>> > > On 20 January 2014 12:26, Dale Harvey <dale@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 burde=
n
>>>>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 th=
e
>>>>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 thin=
k
>>>>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 lea=
d
>>>>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 a=
nd
>> 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 p=
ut
>> 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 litt=
le
>> at a time and save them in between instead of having to devote a block o=
f
>> 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


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