<div dir="ltr">Hi,<div><br></div><div>Before people get too active in removing old stuff - is that always what we want?</div><div><br></div><div>One *good* aspect with anybody being able to upload pretty much anything is that it also serves as a repository others can use to have a look at such a change, comment (even if it's still very far from actually making it into the master branch), or simply sharing development.</div><div><br></div><div>If we want to go the route of only having "prime" patches show up in Gerrit, we also need a strategy for where everything else should go - and then we might end up with a synchronization problem between the two places instead.</div><div><br></div><div>If anything, I would be more inclined to having a hot-list of changes we agree need attention the next few days/weeks (depending on size and complexity), and that this is a list we update jointly e.g. at the telcos (although for large changes it should normally have been preceded by a joint decision it's a needed feature and a discussion about the design)?</div><div><br></div><div>Cheers,</div><div><br></div><div>Erik</div><div><br></div><div><br></div><div><br></div><div><br></div><div> </div><div><br></div><div><br></div><div><br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Feb 19, 2019 at 10:44 PM David van der Spoel <<a href="mailto:spoel@xray.bmc.uu.se">spoel@xray.bmc.uu.se</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Den 2019-02-19 kl. 09:30, skrev Magnus Lundborg:<br>
> Hi,<br>
> <br>
> I'd just like to add that it would be good to get developers used to <br>
> abandoning patches that are no longer worth reviewing/submitting. There <br>
> are some patches in gerrit that are over five years old and quite a few <br>
> that are over three years old. If we want to routinely review the oldest <br>
> changes, which I think is good in general but should definitely not be a <br>
> rule, we really need to make sure that the oldest changes are still <br>
> interesting to submit.<br>
<br>
Interesting discussion. An observation when looking at "My Open Issues" <br>
is that there are quite a few people who have way too many patches open <br>
simultaneously (including myself and other core-developers). This makes <br>
it difficult to keep them all up to date (as Roland noted, by rebasing) <br>
and many patches disappear from the front page and those that are on the <br>
front page have merge conflicts. A patch that is evidently not <br>
up-to-date to me signifies that the developer is not likely to work on <br>
it in the near future and hence there is little point to review the <br>
patch (if I have the competence to do so).<br>
<br>
I started abandoning a couple of patches that were old and removing my <br>
name from others. As a result those ancient patches now top the list. If <br>
other people do the same we can probably clean up the todo list for <br>
everyone?<br>
<br>
Finally, I am one of the culprits generating large patches. Some of <br>
these have gained some approval in the past but not (yet) enough to make <br>
it in. There are likely more such patches by others. We need to find a <br>
way to deal with those as well as small patches.<br>
<br>
<br>
> <br>
> Cheers,<br>
> <br>
> Magnus<br>
> <br>
> On 2019-02-19 09:22, Erik Lindahl wrote:<br>
>> Hi,<br>
>><br>
>> We actually brought that up at our internal gromacs-group kick-off in <br>
>> January (both BioExcel and staff funded by some other projects).<br>
>><br>
>> The first important lesson is that code review can be a relatively <br>
>> major task that in general should be planned already when we start <br>
>> working on a new module/feature/whatever. It takes (and should take <br>
>> time) for several developers, but that also means it's something we <br>
>> need to plan/prioritize based on the overall project needs and what <br>
>> people have funding to work on, so we make it clear that one of the <br>
>> tasks for developers X & Y the next three months is to help reviewing <br>
>> the code for project Z.<br>
>><br>
>> Another important caveat is that we cannot have the combination that <br>
>> anybody can push anything to gerrit, and that also meaning a guarantee <br>
>> that it will eventually make it into the master branch. The cost of <br>
>> allowing anything there is that many patches will never make it (for <br>
>> various reasons), and we might need to get better at separating <br>
>> small/old things that have fallen between chairs from changes where <br>
>> there are no strong incentives for the entire project to include the <br>
>> change (in the sense of all new code/features being added liability <br>
>> for somebody to support)<br>
>><br>
>><br>
>> I would say large changes tend to fall in three categories:<br>
>><br>
>> 1) Things that are on the overall project roadmap, and where we (or <br>
>> somebody else) have funding for people to work on it so it is both a <br>
>> plan for long-term maintenance and other people have stepped up in <br>
>> redmine to help with the review so it is also planned and sync:ed when <br>
>> the changes are coming in. These tend to be reviewed quickly, although <br>
>> the amount of comments/changes required might still mean it takes a <br>
>> while for it to get in.<br>
>><br>
>> 2) Other large changes that are in principle good (say, cleanup), but <br>
>> that might not have been on people's radar. These will sometimes go in <br>
>> within days if they are trivial, but sometimes the can take a very <br>
>> long time to get in. The latter usually because people are busy and <br>
>> their time is already full with things that we agreed had high <br>
>> priority, and there might not be any time to drop those other things <br>
>> short-term just because there's another feature that could also be <br>
>> useful. In my experience, the best way to handle this is to be very <br>
>> active with helping reviewing other people's code and build karma - <br>
>> suddenly those reviews start showing up magically.<br>
>><br>
>> 3) The third category is when somebody who has not been very involved <br>
>> in the project before uploads a large change for a new semi-niche <br>
>> feature, it takes ~50 changes for it to compile cleanly, and then <br>
>> there are another ~50 rebases over the course of a year. Although it <br>
>> might sound tough, for these changes I think the solution increasingly <br>
>> needs to be that they will have to live outside of master branch until <br>
>> (a) there is a clear record of the feature being widely used, (b) <br>
>> somebody continuously involved in the project and reviewing other code <br>
>> takes a responsibility to maintain it. Here we rather send a very bad <br>
>> message if people just start to review any code that shows up in <br>
>> gerrit - IMHO this type of change should NOT be reviewed unless it's <br>
>> the result of if a Redmine thread where we agreed on a plan for <br>
>> supporting it in the releases. That does not mean it's bad code, but <br>
>> until the impact and long-term support is clear, it should not be in <br>
>> master.<br>
>><br>
>><br>
>> So, I like the suggestions of e.g. looking at older changes and/or <br>
>> removing reviewers who don't have time.<br>
>><br>
>> However, I don't see that it will work with a combination of allowing <br>
>> anybody to submit anything, assigning reviewers automatically, and <br>
>> then saying that assignment implies it should be done within a week <br>
>> unless explicitly declined :-) That comes with the risk of spending a <br>
>> lot of time on minor cleanup changes, while we won't have time for the <br>
>> large things people are funded for.<br>
>><br>
>> There I think a better solution is to get better with the social <br>
>> planning at the bi-weekly telcos we have - but rather than merely <br>
>> stating what we are working on right now, we should get better at <br>
>> coming with suggestions ahead of time and then checking (a) if other <br>
>> people also want to prioritize that to help with reviewing, and (b) <br>
>> what other reviewing we can help with.<br>
>><br>
>><br>
>> In principle we'd be delighted to have other people join our internal <br>
>> planning, with two caveats: NDAs means we need to keep some things <br>
>> orthogonal between vendors, and our staff resources are based on the <br>
>> things we have funding to deliver, so if somebody needs help with <br>
>> their feature X, we likely need them to help us review features Y & Z :-)<br>
>><br>
>><br>
>><br>
>> Cheers,<br>
>><br>
>> Erik<br>
>><br>
>><br>
>><br>
>><br>
>><br>
>><br>
>><br>
>><br>
>> On Tue, Feb 19, 2019 at 5:33 AM Schulz, Roland <br>
>> <<a href="mailto:roland.schulz@intel.com" target="_blank">roland.schulz@intel.com</a> <mailto:<a href="mailto:roland.schulz@intel.com" target="_blank">roland.schulz@intel.com</a>>> wrote:<br>
>><br>
>> Hi,<br>
>><br>
>> It seems to me based on my observations (no statistics so likely<br>
>> biased), we have a high inefficiency in that we have certain<br>
>> patches which go months without code review, and the author is<br>
>> forced to regularly rebase because anything not on the first page<br>
>> of gerrit is likely to go unnoticed.<br>
>><br>
>> If that’s true, it seems we should have some way:<br>
>><br>
>> -As an owner to find out whether a change will ever get reviewed<br>
>> or whether the owner should abandon it because it doesn’t have<br>
>> enough interest for it to go in.<br>
>><br>
>> -Have a more efficient way of communicating that changes aren’t<br>
>> abandoned and are in need of code review which doesn’t require<br>
>> regularly rebasing them just to get noticed.<br>
>><br>
>> Based on these observation, I suggestion the following:<br>
>><br>
>> -We should make the reviewer list on a change useful. Currently we<br>
>> have changes which haven’t seen any significant review in months<br>
>> and have a lot of reviewers listed. I suggest, being listed as a<br>
>> reviewer on a change starts to mean one has the intention of<br>
>> reviewing it fully with voting in a timely manner (e.g. within a<br>
>> week for a small change or incremental reviews and within a month<br>
>> for a large initial review). If one is added as a reviewer (either<br>
>> because someone else added one or one has commented in the past)<br>
>> and one doesn’t have sufficient time or interest in the topic to<br>
>> do a full review in a timely manner or doesn’t feel qualified to<br>
>> vote, one should remove oneself from the reviewer list. This would<br>
>> allow us to find out as a reviewer: are there changes which<br>
>> require reviewers and which changes am I expected to review. And<br>
>> as an owner: should I ask for reviews or (after having asked) is<br>
>> there no interest and I might need to abandon the idea.<br>
>><br>
>> -We should use the dashboard to check for incoming reviews rather<br>
>> than primarily using the first page. We should usually start<br>
>> reviewing with the oldest rather the newest changes (requires us<br>
>> to either clean up all the old changes or have a start-date for<br>
>> the policy).<br>
>><br>
>> -We should encourage owners to add others as reviewers to ask them<br>
>> for review (if we notice some people get spammed we might need<br>
>> some suggestions of how to avoid that). This makes it clear if<br>
>> there is no interest if everyone removes themselves. And either<br>
>> lets the owner abandon it or ask for more clarification regarding<br>
>> interest per email.<br>
>><br>
>> -We could potentially improve the dashboard with extensions such<br>
>> as <a href="https://github.com/openstack/gerrit-dash-creator" rel="noreferrer" target="_blank">https://github.com/openstack/gerrit-dash-creator</a><br>
>><br>
>> Do you think a change in the direction would be useful (which<br>
>> should probably decide on the direction before discussing details<br>
>> such as what “timely” means)? Or do have different observations<br>
>> and/or suggestions?<br>
>><br>
>> TLDR: Should every reviewer one a change be expected to review in<br>
>> a timely manner or remove themselves as reviewer from the change?<br>
>><br>
>> Roland<br>
>><br>
>> -- <br>
>> Gromacs Developers mailing list<br>
>><br>
>> * Please search the archive at<br>
>> <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a><br>
>> before posting!<br>
>><br>
>> * Can't post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a><br>
>><br>
>> * For (un)subscribe requests visit<br>
>> <a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a><br>
>> or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a><br>
>> <mailto:<a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>>.<br>
>><br>
>><br>
>><br>
>> -- <br>
>> Erik Lindahl <<a href="mailto:erik.lindahl@dbb.su.se" target="_blank">erik.lindahl@dbb.su.se</a> <mailto:<a href="mailto:erik.lindahl@dbb.su.se" target="_blank">erik.lindahl@dbb.su.se</a>>><br>
>> Professor of Biophysics, Dept. Biochemistry & Biophysics, Stockholm <br>
>> University<br>
>> Science for Life Laboratory, Box 1031, 17121 Solna, Sweden<br>
>><br>
> <br>
> <br>
<br>
<br>
-- <br>
David van der Spoel, Ph.D., Professor of Biology<br>
Head of Department, Cell & Molecular Biology, Uppsala University.<br>
Box 596, SE-75124 Uppsala, Sweden. Phone: +46184714205.<br>
<a href="http://www.icm.uu.se" rel="noreferrer" target="_blank">http://www.icm.uu.se</a><br>
-- <br>
Gromacs Developers mailing list<br>
<br>
* Please search the archive at <a href="http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists/GMX-developers_List</a> before posting!<br>
<br>
* Can't post? Read <a href="http://www.gromacs.org/Support/Mailing_Lists" rel="noreferrer" target="_blank">http://www.gromacs.org/Support/Mailing_Lists</a><br>
<br>
* For (un)subscribe requests visit<br>
<a href="https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers" rel="noreferrer" target="_blank">https://maillist.sys.kth.se/mailman/listinfo/gromacs.org_gmx-developers</a> or send a mail to <a href="mailto:gmx-developers-request@gromacs.org" target="_blank">gmx-developers-request@gromacs.org</a>.</blockquote></div><br clear="all"><div><br></div>-- <br><div dir="ltr" class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr"><div>Erik Lindahl <<a href="mailto:erik.lindahl@dbb.su.se" target="_blank">erik.lindahl@dbb.su.se</a>></div><div>Professor of Biophysics, Dept. Biochemistry & Biophysics, Stockholm University</div><div>Science for Life Laboratory, Box 1031, 17121 Solna, Sweden</div></div></div></div></div></div></div></div></div>