[gmx-developers] Changes without reviews

Tue Feb 19 09:30:54 CET 2019

Hi,

I'd just like to add that it would be good to get developers used to 
abandoning patches that are no longer worth reviewing/submitting. There 
are some patches in gerrit that are over five years old and quite a few 
that are over three years old. If we want to routinely review the oldest 
changes, which I think is good in general but should definitely not be a 
rule, we really need to make sure that the oldest changes are still 
interesting to submit.

Cheers,

Magnus

On 2019-02-19 09:22, Erik Lindahl wrote:
> 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 <mailto: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
>     <mailto:gmx-developers-request at gromacs.org>.
>
>
>
> -- 
> Erik Lindahl <erik.lindahl at dbb.su.se <mailto: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/2b6112a5/attachment-0001.html>