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

[Rich Persaud <persaur@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2990 bytes --]

On Nov 28, 2019, at 09:05, Lars Kurth <lars.kurth@citrix.com> wrote:
> 
> On 28/11/2019, 07:37, "Jan Beulich" <jbeulich@suse.com> 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 reviews 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

Rich


[-- Attachment #1.2: Type: text/html, Size: 4292 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel