Mailing List Archive

?On 28/11/2019, 04:09, "Jan Beulich" <jbeulich@suse.com> wrote:

On 28.11.2019 01:54, Stefano Stabellini wrote:
> On Thu, 26 Sep 2019, Lars Kurth wrote:
>> From: Lars Kurth <lars.kurth@citrix.com>
>>
>> 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


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

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.

Jan

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

?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

Lars

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