<div dir="ltr">Hi,<div><br></div><div>We actually brought that up at our internal gromacs-group kick-off in January (both BioExcel and staff funded by some other projects).</div><div><br></div><div>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.</div><div><br></div><div>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) </div><div><br></div><div><br></div><div>I would say large changes tend to fall in three categories:</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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. </div><div><br></div><div><br></div><div>So, I like the suggestions of e.g. looking at older changes and/or removing reviewers who don't have time.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div><br></div><div>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 :-)</div><div><br></div><div><br></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><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 5:33 AM Schulz, Roland <<a href="mailto:roland.schulz@intel.com">roland.schulz@intel.com</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">
<div lang="EN-US">
<div class="gmail-m_-3647904849388690013WordSection1">
<p class="MsoNormal">Hi,<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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.<u></u><u></u></p>
<p class="MsoNormal">If that’s true, it seems we should have some way:<u></u><u></u></p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><u></u><span>-<span style="font-style:normal;font-variant-caps:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:"Times New Roman"">
</span></span><u></u>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.<u></u><u></u></p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><u></u><span>-<span style="font-style:normal;font-variant-caps:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:"Times New Roman"">
</span></span><u></u>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.<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Based on these observation, I suggestion the following:<u></u><u></u></p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><u></u><span>-<span style="font-style:normal;font-variant-caps:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:"Times New Roman"">
</span></span><u></u>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.<u></u><u></u></p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><u></u><span>-<span style="font-style:normal;font-variant-caps:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:"Times New Roman"">
</span></span><u></u>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). <u></u><u></u></p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><u></u><span>-<span style="font-style:normal;font-variant-caps:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:"Times New Roman"">
</span></span><u></u>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.<u></u><u></u></p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><u></u><span>-<span style="font-style:normal;font-variant-caps:normal;font-weight:normal;font-stretch:normal;font-size:7pt;line-height:normal;font-family:"Times New Roman"">
</span></span><u></u>We could potentially improve the dashboard with extensions such as
<a href="https://github.com/openstack/gerrit-dash-creator" target="_blank">https://github.com/openstack/gerrit-dash-creator</a><u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">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?
<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">TLDR: Should every reviewer one a change be expected to review in a timely manner or remove themselves as reviewer from the change?<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal">Roland<u></u><u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
<p class="MsoNormal"><u></u> <u></u></p>
</div>
</div>
-- <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>