<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 8, 2016 at 7:27 PM, Jeff Darcy <span dir="ltr"><<a href="mailto:jdarcy@redhat.com" target="_blank">jdarcy@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">(combining replies to multiple people)<br>
<br>
Pranith:<br>
<span class="">> I agree about encouraging specific kind of review. At the same time we need<br>
> to make reviewing, helping users in the community as important as sending<br>
> patches in the eyes of everyone. It is very important to know these<br>
> statistics to move in the right direction. My main problem with this is,<br>
> everyone knows that reviews are important, then why are they not happening?<br>
> Is it really laziness?<br>
<br>
</span>"Laziness" was clearly a bad choice of words, for which I apologize. I<br>
should have said "lack of diligence" or something to reflect that it's an<br>
*organizational* rather than personal problem. We *as a group* have not<br>
been keeping up with the review workload. Whatever the reasons are, to<br>
change the outcome we need to change behavior, and to change behavior we<br>
need to change the incentives.<br>
<br>
<br>
Raghavendra G:<br>
<span class="">> Personally I've found a genuine -1 to be more valuable than a +1. Since we<br>
> are discussing about measuring, how does one measure the issues that are<br>
> prevented (through a good design, thoughtful coding/review) than the issues<br>
> that are _fixed_?<br>
<br>
</span>Another excellent point. It's easier to see the failures than the successes.<br>
It's a bit like traffic accidents. Everyone sees when you cause one, but not<br>
when you avoid one. If a regression occurs, everyone can look back to see<br>
who the author and reviewers were. If there's no regression ... what then?<br>
Pranith has suggested some mechanism to give credit/karma in cases where it<br>
can't be done automatically. Meta-review (review of reviews) is another<br>
possibility. I've seen it work in other contexts, but I'm not sure how to<br>
apply it here.<br>
<span class=""><br>
> Measuring -1s and -2s along with +1s and +2s can be a good<br>
> place to start with (though as with many measurements, they may not reflect<br>
> the underlying value accurately).<br>
<br>
</span>The danger here is that we'll incentivize giving a -1 for superficial<br>
reasons. We don't need more patches blocked because a reviewer doesn't<br>
like a file/variable name, or wants to play "I know a better way" games.<br>
Unfortunately, it's hard to distinguish those from enforcing standards<br>
that really matter, or avoiding technical debt. I guess that brings us<br>
back to manual overrides and/or meta-review.<br>
<br>
<br>
Poornima:<br>
<span class="">> Below are the few things that we can do to reduce our review backlog:<br>
> - No time for maintainers to review is not a good enough reason to bitrot<br>
> patches in review for months, it clearly means we need additional<br>
> maintainers for that component?<br>
> - Add maintainers for every component that is in Gluster(atleast the ones<br>
> which have incoming patches)<br>
> - For every patch we submit we add 'component(s)' label, and evaluate if<br>
</span>> gerrit can automatically add maintainers as reviewers, and have another<br>
<span class="">> label 'Maintainers ack' which needs to be present for any patch to be<br>
> merged.<br>
<br>
</span>Excellent points. Not much to add here, except that we also need a way to<br>
deal with patches that cross many components (as many of yours and mine do).<br>
If getting approval from one maintainer is a problem, getting approval from<br>
several will be worse. Maybe it's enough to say that approval by one of<br>
those several maintainers is sufficient, and to rely on maintainers talking<br>
to one another.<br>
<br>
<br>
Atin:<br>
<span class="">> How about having "review marathon" once a week by every team? In past this<br>
> has worked well and I don't see any reason why can't we spend 3-4 hours in a<br>
> meeting on weekly basis to review incoming patches on the component that the<br>
> team owns.<br>
<br>
</span>I love this idea. If I may add to it, I suggest that such "marathons" are a<br>
good way not only to reduce the backlog but also to teach people how to<br>
review well. Reviewing's a skill, learnable like any other. In addition to<br>
improving review quantity, getting reviews more focused on real bugs and<br>
technical debt would be great.<br>
<br>
<br>
Pranith (again):<br>
<span class="">> Everyone in the team started reviewing the patches and giving +1 and I am<br>
> reviewing only after a +1.<br>
<br>
</span>In the past I've done this myself (as a project-level maintainer), so I<br>
totally understand the motivation, but I'm still ambivalent about whether<br>
it's a good idea. On the one hand, it seems like projects bigger than<br>
ours essentially work this way. For example, how often does Linus review<br>
something that hasn't already been reviewed by one of his lieutenants?<br>
Not often, it seems. On the other hand, reinforcing such hierarchies in<br>
the review process is counter to our goal of breaking them down in a more<br>
general sense. I hope some day we can get to the point where people are<br>
actively seeking out things to review, instead of actively filtering the<br>
list they already have.<br></blockquote><div><br></div><div>The feedback I got is, "it is not motivating to review patches that are already merged by maintainer." Do you suggest they should change that behaviour in that case? We are still in discussions about how to change this policy as we missed some patches for 3.8.1 because of this rule. What will lead to people actively seeking out things to review? Did you do anything before that made this happen?<br> In my personal experience, we as a community don't recognize review contribution anywhere. Even the mails say it is maintainers' responsibility to review the patches. And the mails also say these are boring bits of being maintainer. And we have good metrics which show who sends more patches by month/year and it also ranks everyone. So why shouldn't the person aim to get on the charts which has high number of patches sent instead? How many ever mails we send and talk about this over and over nothing will change until reviews/talking to users gets equal recognition. We already showed a stick to measure(<a href="http://projects.bitergia.com/redhat-glusterfs-dashboard/browser/">http://projects.bitergia.com/redhat-glusterfs-dashboard/browser/</a>). Now we are wondering why reviews don't happen. We should fix the measuring sticks.<br><br></div><div>let us give equal recognition for:<br></div><div>patches sent<br></div><div>patches reviewed - this one is missing.<br></div><div>helping users on gluster-users<br></div><div>helping users on #gluster/#gluster-dev<br><br></div><div>Feel free to add anything more I might have missed out. May be new ideas/design/big-refactor?<br><br></div><div>let people do what they like more among these and let us also recognize them for all their contributions. Let us celebrate their work in each monthly news letter.<br><br></div><div>The case I want to make for reviews is that, when you get really good at reviewing, you see the program run on all the machines with all the threads with all possible race conditions/memory leaks/feature bugs/crashes/ etc etc in your head. So no, reviewing is not boring. There could be patches at time which will be boring to review though. At least this has been my personal experience.<br><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<br>
<br>
It's great to see such an energetic discussion. I know it's already<br>
the weekend for everyone I've just replied to, but I hope we can keep<br>
the discussion going next week.<br>
<br>
</blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature" data-smartmail="gmail_signature"><div dir="ltr">Pranith<br></div></div>
</div></div>