[gmx-developers] Changes without reviews

Tue Feb 19 09:22:46 CET 2019

Hi,

We actually brought that up at our internal gromacs-group kick-off in
January (both BioExcel and staff funded by some other projects).

The first important lesson is that code review can be a relatively major
task that in general should be planned already when we start working on a
new module/feature/whatever. It takes (and should take time) for several
developers, but that also means it's something we need to plan/prioritize
based on the overall project needs and what people have funding to work on,
so we make it clear that one of the tasks for developers X & Y the next
three months is to help reviewing the code for project Z.

Another important caveat is that we cannot have the combination that
anybody can push anything to gerrit, and that also meaning a guarantee that
it will eventually make it into the master branch. The cost of allowing
anything there is that many patches will never make it (for various
reasons), and we might need to get better at separating small/old things
that have fallen between chairs from changes where there are no strong
incentives for the entire project to include the change (in the sense of
all new code/features being added liability for somebody to support)

I would say large changes tend to fall in three categories:

1) Things that are on the overall project roadmap, and where we (or
somebody else) have funding for people to work on it so it is both a plan
for long-term maintenance and other people have stepped up in redmine to
help with the review so it is also planned and sync:ed when the changes are
coming in. These tend to be reviewed quickly, although the amount of
comments/changes required might still mean it takes a while for it to get
in.

2) Other large changes that are in principle good (say, cleanup), but that
might not have been on people's radar.  These will sometimes go in within
days if they are trivial, but sometimes the can take a very long time to
get in. The latter usually because people are busy and their time is
already full with things that we agreed had high priority, and there might
not be any time to drop those other things short-term just because there's
another feature that could also be useful. In my experience, the best way
to handle this is to be very active with helping reviewing other people's
code and build karma - suddenly those reviews start showing up magically.

3) The third category is when somebody who has not been very involved in
the project before uploads a large change for a new semi-niche feature, it
takes ~50 changes for it to compile cleanly, and then there are another ~50
rebases over the course of a year. Although it might sound tough, for these
changes I think the solution increasingly needs to be that they will have
to live outside of master branch until (a) there is a clear record of the
feature being widely used, (b) somebody continuously involved in the
project and reviewing other code takes a responsibility to maintain it.
Here we rather send a very bad message if people just start to review any
code that shows up in gerrit - IMHO this type of change should NOT be
reviewed unless it's the result of if a Redmine thread where we agreed on a
plan for supporting it in the releases. That does not mean it's bad code,
but until the impact and long-term support is clear, it should not be in
master.

So, I like the suggestions of e.g. looking at older changes and/or removing
reviewers who don't have time.

However, I don't see that it will work with a combination of allowing
anybody to submit anything, assigning reviewers automatically, and then
saying that assignment implies it should be done within a week unless
explicitly declined :-) That comes with the risk of spending a lot of time
on minor cleanup changes, while we won't have time for the large things
people are funded for.

There I think a better solution is to get better with the social planning
at the bi-weekly telcos we have - but rather than merely stating what we
are working on right now, we should get better at coming with suggestions
ahead of time and then checking (a) if other people also want to prioritize
that to help with reviewing, and (b) what other reviewing we can help with.

In principle we'd be delighted to have other people join our internal
planning, with two caveats: NDAs means we need to keep some things
orthogonal between vendors, and our staff resources are based on the things
we have funding to deliver, so if somebody needs help with their feature X,
we likely need them to help us review features Y & Z :-)

Cheers,

Erik

On Tue, Feb 19, 2019 at 5:33 AM Schulz, Roland <roland.schulz at intel.com>
wrote:

> Hi,
>
>
>
> It seems to me based on my observations (no statistics so likely biased),
> we have a high inefficiency in that we have certain patches which go months
> without code review, and the author is forced to regularly rebase because
> anything not on the first page of gerrit is likely to go unnoticed.
>
> If that’s true, it seems we should have some way:
>
> -          As an owner to find out whether a change will ever get
> reviewed or whether the owner should abandon it because it doesn’t have
> enough interest for it to go in.
>
> -          Have a more efficient way of communicating that changes aren’t
> abandoned and are in need of code review which doesn’t require regularly
> rebasing them just to get noticed.
>
>
>
> Based on these observation, I suggestion the following:
>
> -          We should make the reviewer list on a change useful. Currently
> we have changes which haven’t seen any significant review in months and
> have a lot of reviewers listed. I suggest, being listed as a reviewer on a
> change starts to mean one has the intention of reviewing it fully with
> voting in a timely manner (e.g. within a week for a small change or
> incremental reviews and within a month for a large initial review). If one
> is added as a reviewer (either because someone else added one or  one has
> commented in the past) and one doesn’t have sufficient time or interest in
> the topic to do a full review in a timely manner or doesn’t feel qualified
> to vote, one should remove oneself from the reviewer list. This would allow
> us to find out as a reviewer: are there changes which require reviewers and
> which changes am I expected to review. And as an owner: should I ask for
> reviews or (after having asked) is there no interest and I might need to
> abandon the idea.
>
> -          We should use the dashboard to check for incoming reviews
> rather than primarily using the first page. We should usually start
> reviewing with the oldest rather the newest changes (requires us to either
> clean up all the old changes or have a start-date for the policy).
>
> -          We should encourage owners to add others as reviewers to ask
> them for review (if we notice some people get spammed we might need some
> suggestions of how to avoid that). This makes it clear if there is no
> interest if everyone removes themselves. And either lets the owner abandon
> it or ask for more clarification regarding interest per email.
>
> -          We could potentially improve the dashboard with extensions
> such as https://github.com/openstack/gerrit-dash-creator
>
>
>
> Do you think a change in the direction would be useful (which should
> probably decide on the direction before discussing details such as what
> “timely” means)? Or do have different observations and/or suggestions?
>
>
>
> TLDR: Should every reviewer one a change be expected to review in a timely
> manner or remove themselves as reviewer from the change?
>
>
>
> Roland
>
>
>
>
> --
> Gromacs Developers mailing list
>
> * Please search the archive at
> http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List before
> posting!
>
> * Can't post? Read http://www.gromacs.org/Support/Mailing_Lists
>
> * For (un)subscribe requests visit
> https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers
> or send a mail to gmx-developers-request at gromacs.org.

-- 
Erik Lindahl <erik.lindahl at dbb.su.se>
Professor of Biophysics, Dept. Biochemistry & Biophysics, Stockholm
University
Science for Life Laboratory, Box 1031, 17121 Solna, Sweden
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://maillist.sys.kth.se/pipermail/gromacs.org_gmx-developers/attachments/20190219/9147a364/attachment-0001.html>