<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body text="#000000" bgcolor="#FFFFFF">
<div class="moz-cite-prefix">Hi,</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">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.</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Cheers,</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">Magnus<br>
</div>
<div class="moz-cite-prefix"><br>
</div>
<div class="moz-cite-prefix">On 2019-02-19 09:22, Erik Lindahl
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CAEJJM8FkJfxhJBWi=ETeG5sPwZh9KZhrBE2YGND0fC0AQ55-XQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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"
moz-do-not-send="true">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,</p>
<p class="MsoNormal"> </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.</p>
<p class="MsoNormal">If that’s true, it seems we should
have some way:</p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><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>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.</p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><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>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.</p>
<p class="MsoNormal"> </p>
<p class="MsoNormal">Based on these observation, I
suggestion the following:</p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><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>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.</p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><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>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). </p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><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>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.</p>
<p class="gmail-m_-3647904849388690013MsoListParagraph"><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>We could potentially improve the
dashboard with extensions such as
<a
href="https://github.com/openstack/gerrit-dash-creator"
target="_blank" moz-do-not-send="true">https://github.com/openstack/gerrit-dash-creator</a></p>
<p class="MsoNormal"> </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?
</p>
<p class="MsoNormal"> </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?</p>
<p class="MsoNormal"> </p>
<p class="MsoNormal">Roland</p>
<p class="MsoNormal"> </p>
<p class="MsoNormal"> </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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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" moz-do-not-send="true">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>
<br>
<fieldset class="mimeAttachmentHeader"></fieldset>
</blockquote>
<p><br>
</p>
</body>
</html>