Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

[Lars Kurth <lars.kurth@xxxxxxxxxx>]

From: Lars Kurth <lars.kurth@xxxxxxxxxx>
Date: Thursday, 28 November 2019 at 19:39
To: Rich Persaud <persaur@xxxxxxxxx>
Cc: 'Jan Beulich' <JBeulich@xxxxxxxx>, "lars.kurth@xxxxxxxxxxxxxx" 
<lars.kurth@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, 
"xen-api@xxxxxxxxxxxxxxxxxxxx" <xen-api@xxxxxxxxxxxxxxxxxxxx>, 
"minios-devel@xxxxxxxxxxxxxxxxxxxx" <minios-devel@xxxxxxxxxxxxxxxxxxxx>, 
"committers@xxxxxxxxxxxxxx" <committers@xxxxxxxxxxxxxx>, 
"mirageos-devel@xxxxxxxxxxxxxxxxxxxx" <mirageos-devel@xxxxxxxxxxxxxxxxxxxx>, 
xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "win-pv-devel@xxxxxxxxxxxxxxxxxxxx" 
<win-pv-devel@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide

 
 
From: Rich Persaud <persaur@xxxxxxxxx>
Date: Thursday, 28 November 2019 at 12:21
To: Lars Kurth <lars.kurth@xxxxxxxxxx>
Cc: 'Jan Beulich' <JBeulich@xxxxxxxx>, "lars.kurth@xxxxxxxxxxxxxx" 
<lars.kurth@xxxxxxxxxxxxxx>, Stefano Stabellini <sstabellini@xxxxxxxxxx>, 
"xen-api@xxxxxxxxxxxxxxxxxxxx" <xen-api@xxxxxxxxxxxxxxxxxxxx>, 
"minios-devel@xxxxxxxxxxxxxxxxxxxx" <minios-devel@xxxxxxxxxxxxxxxxxxxx>, 
"committers@xxxxxxxxxxxxxx" <committers@xxxxxxxxxxxxxx>, 
"mirageos-devel@xxxxxxxxxxxxxxxxxxxx" <mirageos-devel@xxxxxxxxxxxxxxxxxxxx>, 
xen-devel <xen-devel@xxxxxxxxxxxxxxxxxxxx>, "win-pv-devel@xxxxxxxxxxxxxxxxxxxx" 
<win-pv-devel@xxxxxxxxxxxxxxxxxxxx>
Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide
 
On Nov 28, 2019, at 09:05, Lars Kurth <lars.kurth@xxxxxxxxxx> wrote:
 
On 28/11/2019, 07:37, "Jan Beulich" <jbeulich@xxxxxxxx> wrote:

   On 28.11.2019 14:06, Lars Kurth wrote:


I can certainly add something on the timing , along the lines of
* For complex series, consider the time it takes to do reviews (maybe with a 
guide of LOC per hour) and give reviewers enough time to
* For series with design issues or large questions, try and highlight the key 
open issues in cover letters clearly and solicit feedback from key maintainers 
who can comment on the open issue. The idea is to save both the contributor and 
the reviewers time by focussing on what needs to be resolved 
* Don’t repost a series, unless all review comments are addressed
or the reviewers asked you to do so. The problem with this is that
this is somewhat in conflict with the "let's focus on the core
issues and not get distracted by details early on in a review cycle".
In other words, this can only work, if reviewers focus on major
issues in early reviews only and do not focus on style, coding
standards, etc.

   But this doesn't make much sense either, because then full re-reviews
   need to happen anyway on later versions, to also deal with the minor
   issues. For RFC kind of series omitting style and alike feedback
   certainly makes sense, but as soon as a patch is non-RFC, it should
   be considered good to go in by the submitter.

OK, I think we have a disconnect between ideal and reality. 

I see two issues today
* Key maintainers don't always review RFC series [they end up at the bottom of 
the priority list, even though spending time on RFCs will save time elsewhere 
later]. So the effect is that then the contributor assumes there are no major 
issues and ends it as a proper series
* In practice what has happened often in the past is that design, architecture, 
assumption flaws are found in early versions of a series.
  - This usually happens because of an oversight or because there was no design 
discussion prior to the series being posted and agreed
  - Common sense would dictate that the biggest benefit for both the reviewer, 
the contributor and the community as a whole would be to try and focus on such 
flaws and leave everything aside
  - Of course there may be value in doing a detailed review of parts of such a 
series as there may be bits that are unaffected by such a flaw
  - But there will likely be parts which are not: doing a detailed review of 
such portions wastes everyone's time

So coming back to your point. Ideally, it would be nice if we had the 
capability to call out parts of a series as "problematic" and treating such 
parts differently.
 
  We may be able to reuse some "Shift Left" terminology, including citations of 
previous Xen code reviews to illustrate categories of design issues that can be 
shifted left:
  
    https://devopedia.org/shift-left
  
I like that idea. We seem to not have come to a conclusion on this specific 
topic, but maybe for now it is sufficient to call this out as a potential issue 
in the guide.
 
Before I send out a new version, it would be good to get at least Jan’s view on 
the issue. 
 
Lars

I have a draft version of  this series ready, but wanted to check how some of 
it resonates. Also, I do have open questions, where I am looking for input from 
seasoned reviewers

I propose to add the following section to code-review-guide.md

----
## <a name="problems"></a>Problematic Patch Reviews

A typical waterfall software development process is sequential with the 
following 
steps: define requirements, analyse, design, code, test and deploy. Problems 
uncovered by code review or testing at such a late stage can cause costly 
redesign 
and delays. The principle of **[Shift Left](https://devopedia.org/shift-left)** 
is to take a 
task that is traditionally performed at a late stage in the process and perform 
that task 
at earlier stages. The goal is to save time by avoiding refactoring.

Typically, problematic patch reviews uncover issues such as wrong or missed 
assumptions, a problematic architecture or design, or other bugs that require 
significant re-implementation of a patch series to fix the issue.

The principle of **Shift Left** also applies in code reviews. Let's assume a 
series has
a major flaw: ideally, this flaw would be picked up in the **first or second 
iteration** of 
the code review. As significant parts of the code may have to be re-written, it 
does not 
make sense for reviewers to highlight minor issues (such as style issues) until 
major 
flaws have been addressed. By providing feedback on minor issues reviewers 
cause 
the code author and themselves extra work by asking for changes to code, which 
ultimately may be changed later.

The question then becomes, how do code reviewers identify major issues early? 
----
This is where I really need help. Are there any tips and recommendations that 
we could give?
I can clearly highlight that we have RFC series, but in practice that does not 
solve the problem as RFCs don’t get prioritized
How do reviewers normally approach a series: do you a) take a big picture view 
first, or b) do most of you work through a series sequentially

I then propose to change the following section in communication-practice.md
----
### Prioritize significant flaws
If a patch or patch series has significant flaws, such as
* It is built on wrong assumptions
* There are issues with the architecture or the design

it does not make sense to do a detailed code review. In such cases, it is best 
to
focus on the major issues first and deal with style and minor issues in a 
subsequent
review. Not all series have significant flaws, but most series have different 
classes of 
changes that are required for acceptance: covering a range of major code 
modifications to minor code style fixes. To avoid misunderstandings between 
reviewers and contributors, it is important to establish and agree whether a 
series or 
part of a series has a significant flaw and agree a course of action. 

A pragmatic approach would be to
* Highlight problematic portions of a series in the cover letter 
* For the patch author and reviewer(s) to agree that for problematic to omit 
style and
minor issues in the review, until the significant flaw is addressed

This saves both the patch author and reviewer(s) time. Note that some background
is covered in detail in [Problematic Patch 
Reviews](resolving-disagreement.md#problems).
----
I think is a pragmatic approach that addresses some of Jan's concerns

Best Regards
Lars


_______________________________________________
MirageOS-devel mailing list
MirageOS-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/mirageos-devel