Mailing-List: contact dev-help@asterixdb.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@asterixdb.apache.org
Subject: Re: [DISCUSS] Improving reviews
To: dev@asterixdb.apache.org
References: <D14919E0-405E-458A-AE96-425C5490ECF6@apache.org>
 <36DF04AB-7D48-45E8-A7ED-10C51DD4DD19@apache.org>
 <CAN_YF5yfVS9UPKzixnuvShM_Fq3fuxUZS6RGsCPRnSjttn6q0Q@mail.gmail.com>
 <CAFUTFebjeSyoeAVaeo3xqH3A0KcZ4FcF-dvZ8sKz8m9b5kO23A@mail.gmail.com>
From: Mike Carey <dtabass@gmail.com>
Message-ID: <96ebe4f5-78aa-8175-db12-429c7b471556@gmail.com>
Date: Tue, 13 Dec 2016 09:38:32 -0800
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.11; rv:45.0)
 Gecko/20100101 Thunderbird/45.5.1
MIME-Version: 1.0
In-Reply-To: <CAFUTFebjeSyoeAVaeo3xqH3A0KcZ4FcF-dvZ8sKz8m9b5kO23A@mail.gmail.com>
Content-Type: multipart/alternative;
 boundary="------------1ABDDDBBF7935CF1C0C2E7DD"
archived-at: Tue, 13 Dec 2016 17:38:44 -0000

--------------1ABDDDBBF7935CF1C0C2E7DD
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 8bit

+1 on 72 hours as a strong guidelined as well. My thoughts are:

1. Code reviews are important - IMO we should most certainly not do away 
with them.  This is especially true, I think, in a world where a number 
of the contributors are grad students.  In addition to making sure that 
all new code is reasonably "industrial strength" (for an open source 
definition of industry), it's probably very valuable to their 
longer-term education.

2. 72 hours will only be feasible (or reasonable) if people follow the 
guidelines that are probably too often ignored - i.e., people need to be 
more self-checking.  If something can be done independent of something 
else, do NOT bundle those things.  If you are doing some refactoring as 
part of a big technical change, first do the refactoring - then do the 
change separately.  Changes involving large numbers of lines of code 
throughout the system should almost "never" be complex technical 
changes.  And if you haven't first addressed the automated comments, or 
test failures - don't put your stuff up for review with people 
attached.  (And if someone asks you to review something that's broken in 
one of those ways, give it a fast negative review and move on; your time 
should not be wasted on ill-prepared changes.)

3. We should indeed set up an automated e-mail that makes all too-long 
reviews' statuses visible.  However, exceeding 72 hours should NOT 
default to acceptance - that would defeat reviews - it should simply 
lead to fair game to increasingly/publically harass the slow reviewer. :-)

4. If you aren't going to be able to review something, speak up right 
away - don't just let things sit.

5. For substantial technical changes/additions, it would be nice somehow 
(suggestions?) to have their review requests be accompanied by the 
provision of a short design document overview-ing the change's design.

Cheers (from a Dilbert-manager-like guy on the team),

Mike

On 12/12/16 10:17 PM, Michael Blow wrote:
> +1 on 72h- seems reasonable.
>
> The forthcoming reporting can clearly indicate the age of reviews and if
> they are approaching / exceeding the SLA.
>
> I think it would be good to get code coverage information integrated with
> the reviews- it seems helpful to know if added / modified code is being
> exercised by tests.
>
> On Wed, Dec 7, 2016 at 12:25 PM Ian Maxon <imaxon@uci.edu> wrote:
>
>> I think that if we can all agree on what a reasonable upper-bound timeframe
>> is for addressing reviews and comments is, it'd help a lot. 72h seems fine
>> to me. I think that we all have been on either side of having a review held
>> up and it can get aggravating and draw things out un-necessarily.
>>
>> On Wed, Dec 7, 2016 at 7:31 AM, Murtadha Hubail <mhubail@apache.org>
>> wrote:
>>
>>> I’m for any type of improvement of the current code review process.
>>> Currently, I have only few hours a week to contribute to the project and
>>> almost every week I start working on a new change. As the number of
>> touched
>>> classes on the change increases, I just decide to abandon it and start
>>> looking for another change that might be smaller. I do this because I
>> know
>>> the bigger change will be stuck in the code review queue and when that
>>> happens I feel discouraged from contributing and my contribution doesn’t
>>> add a value to the system.
>>>
>>> Even though the proposed idea might not solve all the issues in the
>>> current code review process, I believe it is definitely going to improve
>> it
>>> (and encourage more contributions).
>>>
>>> Cheers,
>>> Murtadha
>>>> On Dec 6, 2016, at 9:49 AM, Till Westmann <tillw@apache.org> wrote:
>>>>
>>>> Hi,
>>>>
>>>> today a few of us had a discussion about how we could make the
>> reviewing
>>>> process moving along a little smoother. The goal is to increase the
>>> likeliness
>>>> that the reviews and review comments get addressed reasonably quickly.
>>> To do
>>>> that, the proposal is to
>>>> a) try to keep ourselves to some time limit up to which a reviewer or
>>> author
>>>>    responds to a review or a comment and to
>>>> a) regularly report via e-mail about open reviews and how long they
>> have
>>> been
>>>>    open (Ian already has filed an issue to automate this [1]).
>>>> Of course one is not always able to spend the time to do a thorough
>>> review [2]
>>>> / respond fully to comments, but in this case we should aim to let the
>>> other
>>>> participants know within the time limit that the task is not feasible
>> so
>>> that
>>>> they adapt their plan accordingly.
>>>> The first proposal for the time limit would be 72h (which is taken from
>>> the
>>>> minimal time that a [VOTE] stays open to allow people in all different
>>>> locations and timezones to vote).
>>>> Another goal would be to abandon reviews, if nobody seems to be working
>>> on them
>>>> for a while (and we’d need to find out what "a while" could be).
>>>>
>>>> Thoughts on this?
>>>> A good idea or too much process?
>>>> Is the time limit reasonable?
>>>> Please let us know what you think (ideally more than a +1 or a -1 ...)
>>>>
>>>> Cheers,
>>>> Till
>>>>
>>>> [1] https://issues.apache.org/jira/browse/ASTERIXDB-1745
>>>> [2] https://cwiki.apache.org/confluence/display/ASTERIXDB/Code+Reviews
>>>


--------------1ABDDDBBF7935CF1C0C2E7DD--