Re: [Xen-devel] [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide
From: Lars Kurth Date: Thursday, 28 November 2019 at 19:39 To: Rich Persaud Cc: 'Jan Beulich' , "lars.ku...@xenproject.org" , Stefano Stabellini , "xen-...@lists.xenproject.org" , "minios-de...@lists.xenproject.org" , "committ...@xenproject.org" , "mirageos-de...@lists.xenproject.org" , xen-devel , "win-pv-de...@lists.xenproject.org" Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide From: Rich Persaud Date: Thursday, 28 November 2019 at 12:21 To: Lars Kurth Cc: 'Jan Beulich' , "lars.ku...@xenproject.org" , Stefano Stabellini , "xen-...@lists.xenproject.org" , "minios-de...@lists.xenproject.org" , "committ...@xenproject.org" , "mirageos-de...@lists.xenproject.org" , xen-devel , "win-pv-de...@lists.xenproject.org" Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide On Nov 28, 2019, at 09:05, Lars Kurth wrote: On 28/11/2019, 07:37, "Jan Beulich" 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 ## 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
Re: [Xen-devel] [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide
From: Rich Persaud Date: Thursday, 28 November 2019 at 12:21 To: Lars Kurth Cc: 'Jan Beulich' , "lars.ku...@xenproject.org" , Stefano Stabellini , "xen-...@lists.xenproject.org" , "minios-de...@lists.xenproject.org" , "committ...@xenproject.org" , "mirageos-de...@lists.xenproject.org" , xen-devel , "win-pv-de...@lists.xenproject.org" Subject: Re: [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide On Nov 28, 2019, at 09:05, Lars Kurth wrote: On 28/11/2019, 07:37, "Jan Beulich" 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
Re: [Xen-devel] [MirageOS-devel] [PATCH v2 4/6] Add Code Review Guide
On Nov 28, 2019, at 09:05, Lars Kurth wrote: > > On 28/11/2019, 07:37, "Jan Beulich" 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 ___ Xen-devel mailing list Xen-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/xen-devel
