Xen project Mailing List

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