Re: [MirageOS-devel] [Xen-devel] [PATCH v2 4/6] Add Code Review Guide
On 28/11/2019, 12:12, "Rich Persaud" wrote: On Nov 28, 2019, at 05:12, Jan Beulich wrote: > > On 28.11.2019 01:54, Stefano Stabellini wrote: >>> On Thu, 26 Sep 2019, Lars Kurth wrote: >>> From: Lars Kurth >>> >>> This document highlights what reviewers such as maintainers and committers look >>> for when reviewing code. It sets expectations for code authors and provides >>> a framework for code reviewers. >> >> I think the document is missing a couple of things: >> >> - a simple one line statement that possibly the most important thing in >> a code review is to indentify any bugs in the code >> >> - an explanation that requests for major changes to the series should be >> made early on (i.e. let's not change the architecture of a feature at >> v9 if possible) I also made this comment in reply to patch #5. I'll >> let you decide where is the best place for it. > > This needs balancing. People crucial to the evaluation of a new > feature and its implementation simply may not have the time to > reply prior to v9. We've had situations where people posted new > revisions every other day, sometimes even more than one per day. > > As indicated in several other contexts before - imo people not > helping to shoulder the review load should also not have the > expectation that their (large) contributions will be looked at > in due course. To make this actionable, we could have: - reviewer demand index: automated index of open patches still in need of review, sorted by decreasing age - review flow control: each new patch submission cites one recent review by the patch submitter, for a patch of comparable size - reviewer supply growth: a bootstrapping guide for new reviewers and submitters, with patterns, anti-patterns, and examples to be emulated That is a great idea. However, I would not want to hold up the publication of these documents on these suggestions. Some of them would require implementing tools. I was hoping there would be more progress on lore and others tooling/workflow related stuff by now. So I think for now, I think it is sufficient to set expectations better. Regards Lars ___ MirageOS-devel mailing list MirageOS-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/mirageos-devel
Re: [MirageOS-devel] [Xen-devel] [PATCH v2 4/6] Add Code Review Guide
On Thu, 28 Nov 2019, Jan Beulich wrote: > On 28.11.2019 01:54, Stefano Stabellini wrote: > > On Thu, 26 Sep 2019, Lars Kurth wrote: > >> From: Lars Kurth > >> > >> This document highlights what reviewers such as maintainers and committers > >> look > >> for when reviewing code. It sets expectations for code authors and provides > >> a framework for code reviewers. > > > > I think the document is missing a couple of things: > > > > - a simple one line statement that possibly the most important thing in > > a code review is to indentify any bugs in the code > > > > - an explanation that requests for major changes to the series should be > > made early on (i.e. let's not change the architecture of a feature at > > v9 if possible) I also made this comment in reply to patch #5. I'll > > let you decide where is the best place for it. > > This needs balancing. People crucial to the evaluation of a new > feature and its implementation simply may not have the time to > reply prior to v9. We've had situations where people posted new > revisions every other day, sometimes even more than one per day. Yes, you are right, it needs balancing. This is not meant to encourage contributors to send 9 versions of a series within a week or two :-) We could say that "contributors should make sure to give enough time to all the key stakeholders to review the series". > As indicated in several other contexts before - imo people not > helping to shoulder the review load should also not have the > expectation that their (large) contributions will be looked at > in due course. I think you are right on this point, and maybe we could add something to that effect ___ MirageOS-devel mailing list MirageOS-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/mirageos-devel
Re: [MirageOS-devel] [Xen-devel] [PATCH v2 4/6] Add Code Review Guide
On Nov 28, 2019, at 05:12, Jan Beulich wrote: > > On 28.11.2019 01:54, Stefano Stabellini wrote: >>> On Thu, 26 Sep 2019, Lars Kurth wrote: >>> From: Lars Kurth >>> >>> This document highlights what reviewers such as maintainers and committers >>> look >>> for when reviewing code. It sets expectations for code authors and provides >>> a framework for code reviewers. >> >> I think the document is missing a couple of things: >> >> - a simple one line statement that possibly the most important thing in >> a code review is to indentify any bugs in the code >> >> - an explanation that requests for major changes to the series should be >> made early on (i.e. let's not change the architecture of a feature at >> v9 if possible) I also made this comment in reply to patch #5. I'll >> let you decide where is the best place for it. > > This needs balancing. People crucial to the evaluation of a new > feature and its implementation simply may not have the time to > reply prior to v9. We've had situations where people posted new > revisions every other day, sometimes even more than one per day. > > As indicated in several other contexts before - imo people not > helping to shoulder the review load should also not have the > expectation that their (large) contributions will be looked at > in due course. To make this actionable, we could have: - reviewer demand index: automated index of open patches still in need of review, sorted by decreasing age - review flow control: each new patch submission cites one recent review by the patch submitter, for a patch of comparable size - reviewer supply growth: a bootstrapping guide for new reviewers and submitters, with patterns, anti-patterns, and examples to be emulated Rich ___ MirageOS-devel mailing list MirageOS-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/mirageos-devel
Re: [MirageOS-devel] [Xen-devel] [PATCH v2 4/6] Add Code Review Guide
On 28/11/2019, 04:09, "Jan Beulich" wrote: On 28.11.2019 01:54, Stefano Stabellini wrote: > On Thu, 26 Sep 2019, Lars Kurth wrote: >> From: Lars Kurth >> >> This document highlights what reviewers such as maintainers and committers look >> for when reviewing code. It sets expectations for code authors and provides >> a framework for code reviewers. > > I think the document is missing a couple of things: > > - a simple one line statement that possibly the most important thing in > a code review is to indentify any bugs in the code > > - an explanation that requests for major changes to the series should be > made early on (i.e. let's not change the architecture of a feature at > v9 if possible) I also made this comment in reply to patch #5. I'll > let you decide where is the best place for it. This needs balancing. People crucial to the evaluation of a new feature and its implementation simply may not have the time to reply prior to v9. We've had situations where people posted new revisions every other day, sometimes even more than one per day. 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. As soon as a reviewer comes back with detailed feedback, the contributor will feel obliged to fix these. This creates a motivation to want to please the reviewer send out new versions of series fixing cosmetic issues without addressing the substantial issues, leading to what Jan describes. I am looking for opinions here. As indicated in several other contexts before - imo people not helping to shoulder the review load should also not have the expectation that their (large) contributions will be looked at in due course. I can add something to this effect. Lars ___ MirageOS-devel mailing list MirageOS-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/mirageos-devel
Re: [MirageOS-devel] [Xen-devel] [PATCH v2 4/6] Add Code Review Guide
On 28.11.2019 01:54, Stefano Stabellini wrote: > On Thu, 26 Sep 2019, Lars Kurth wrote: >> From: Lars Kurth >> >> This document highlights what reviewers such as maintainers and committers >> look >> for when reviewing code. It sets expectations for code authors and provides >> a framework for code reviewers. > > I think the document is missing a couple of things: > > - a simple one line statement that possibly the most important thing in > a code review is to indentify any bugs in the code > > - an explanation that requests for major changes to the series should be > made early on (i.e. let's not change the architecture of a feature at > v9 if possible) I also made this comment in reply to patch #5. I'll > let you decide where is the best place for it. This needs balancing. People crucial to the evaluation of a new feature and its implementation simply may not have the time to reply prior to v9. We've had situations where people posted new revisions every other day, sometimes even more than one per day. As indicated in several other contexts before - imo people not helping to shoulder the review load should also not have the expectation that their (large) contributions will be looked at in due course. Jan ___ MirageOS-devel mailing list MirageOS-devel@lists.xenproject.org https://lists.xenproject.org/mailman/listinfo/mirageos-devel
