[gmx-developers] Changes without reviews

Tue Feb 19 22:44:46 CET 2019

Den 2019-02-19 kl. 09:30, skrev Magnus Lundborg:
> 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.

Interesting discussion. An observation when looking at "My Open Issues" 
is that there are quite a few people who have way too many patches open 
simultaneously (including myself and other core-developers). This makes 
it difficult to keep them all up to date (as Roland noted, by rebasing) 
and many patches disappear from the front page and those that are on the 
front page have merge conflicts. A patch that is evidently not 
up-to-date to me signifies that the developer is not likely to work on 
it in the near future and hence there is little point to review the 
patch (if I have the competence to do so).

I started abandoning a couple of patches that were old and removing my 
name from others. As a result those ancient patches now top the list. If 
other people do the same we can probably clean up the todo list for 
everyone?

Finally, I am one of the culprits generating large patches. Some of 
these have gained some approval in the past but not (yet) enough to make 
it in. There are likely more such patches by others. We need to find a 
way to deal with those as well as small patches.

> 
> 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
>>
> 
> 

-- 
David van der Spoel, Ph.D., Professor of Biology
Head of Department, Cell & Molecular Biology, Uppsala University.
Box 596, SE-75124 Uppsala, Sweden. Phone: +46184714205.
http://www.icm.uu.se