+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 <[email protected]> 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 <[email protected]>
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 <[email protected]> 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
