From: Hanchett, Paul [mailto:[email protected]] Sent: Friday, January 31, 2014 9:29 AM To: Schaufler, Casey Cc: Łukasz Stelmach; Philippe Coval; [email protected] Subject: Re: [Dev] State of Pending changes , how to enhance workflow ? ( Was Dear Maintainers )
I thought it took approval of two reviewers for a change to be accepted. That's not quite the same as using a pool of reviewers to ensure that *someone* has time to review the change... That is correct. It is the process that is currently not working. Re: "dedicated reviewers" -- Isn't supposed to be peer review? If the reviewers are dedicated, then you lose the peer part. Also, it would take more than two people full time to get the review done. In my experience, a proper review can take as long to do as writing the original code... That’s true in many cases, but I see an awful lot of packaging and configuration changes getting backlogged. Generally it’s not *code* reviews that are the problem, it is packaging and the like. There’s also no reason that dedicated reviewers shouldn’t have the authority to delegate individual reviews to others. In that case the dedicated reviewer has to approve a change based on that reviewers comment. Another alternative is that we require a Reviewed-by: line just like we require a Signed-off-by: and a Change-id: when a change is submitted. The dedicated reviewers could decide if that was good enough and would have some assurance that the change had been run past someone. Of course, “Reviewed-by:” can’t be the submitter. Of course, there's always the possibility that a change hasn't been approved because while no one wants to vote it down, no one believes in the change enough to put their reputation on the line for it. :-\ All the more reason for a “Reviewed-by:”. If no one is willing to put their name on the line, the change shouldn’t be submitted. I've notice several instances where someone not only submitted a change but they also +1'ed as a reviewer. While I admire their self confidence, I think that practice defeats the purpose of the review. I agree. > If they don’t know Bluetooth, WRT or the Linux kernel that’s OK. > Engineers are supposed to be smart, and they can learn. The same can be said of a manager. Would you trust code reviewed by a manager? As you might expect, that depends on the manager. I’ve been a manager, and it didn’t diminish my review skills all that much. My point is that subject matter expertise is important because if the defects were obvious enough for a beginner to catch, they would not have been left in the code in the first place! FWIW! Paul Paul Hanchett ------------------- Infotainment Engineer MSX on behalf of Jaguar Land Rover One World Trade Center, 121 Southwest Salmon Street, 11th Floor, Portland, Oregon, 97204 Email: [email protected]<mailto:[email protected]> ------------------- Business Details: Jaguar Land Rover Limited Registered Office: Abbey Road, Whitley, Coventry CV3 4LF Registered in England No: 1672070 On Fri, Jan 31, 2014 at 8:33 AM, Schaufler, Casey <[email protected]<mailto:[email protected]>> wrote: The correct number of reviewers is 2. Not per project, total. If you dedicate 2 people to the review process (it’s possible but unlikely that they would be able to do anything else while so encumbered) you get accountability and timely response. If they don’t know Bluetooth, WRT or the Linux kernel that’s OK. Engineers are supposed to be smart, and they can learn. If they have questions, they can ask them. From: [email protected]<mailto:[email protected]> [mailto:[email protected]<mailto:[email protected]>] On Behalf Of Hanchett, Paul Sent: Friday, January 31, 2014 8:01 AM To: Łukasz Stelmach Cc: Philippe Coval; [email protected]<mailto:[email protected]> Subject: Re: [Dev] State of Pending changes , how to enhance workflow ? ( Was Dear Maintainers ) As a Management Principle, a smaller group tends to act with more personal accountablility than does a larger one. One management guru says it thus "When everyone is responsible <for something>, then no one is." I'd suggest that even 10 reviewers might be too many. I would also think that a reviewer needs to be a SME (subject matter expert); I'm not sure how you'd bring that into the mix, but it could be important from a quality control perspective. Not sure that's worth even $0.02, but it's what I have to contribute. <g> Paul Paul Hanchett ------------------- Infotainment Engineer MSX on behalf of Jaguar Land Rover One World Trade Center, 121 Southwest Salmon Street, 11th Floor, Portland, Oregon, 97204 Email: [email protected]<mailto:[email protected]> ------------------- Business Details: Jaguar Land Rover Limited Registered Office: Abbey Road, Whitley, Coventry CV3 4LF Registered in England No: 1672070 On Fri, Jan 31, 2014 at 1:25 AM, Łukasz Stelmach <[email protected]<mailto:[email protected]>> wrote: It was <2014-01-30 czw 23:53>, when Thiago Macieira wrote: > On quinta-feira, 30 de janeiro de 2014 21:54:20, Patrick Ohly wrote: >> On Thu, 2014-01-30 at 10:48 -0800, Thiago Macieira wrote: >> > Ensuring that everything got reviewed is the responsibility of the >> > maintainer. But it's the *interest* of the contributor, so the >> > contributor should pay attention to the state of the contribution. >> > >> > If a contribution goes unreviewed after a while, ping the reviewers. >> >> That's impractical when the list of reviewers is larger than a handful. >> Who should get contacted? Randomly? Chances are high that the ones not >> doing any active work on the package will get pinged, annoying them >> further and causing additional delays. > > Just write a new comment saying "ping". > > Any of the reviewers is capable of approving the change. The question is only > whether they feel confident in doing so. > >> What's your take on those packages which currently have more than ten >> reviewers? > > Having that many reviewers is great! That means more people to look at > the changes, to find potential issues, offer advice as well as to > provide round-the- clock ability to review things. Having too few > reviewers (or none) is a bad thing -- for example, for my changes in > QtCore, very often no one can review them, which means I need to seek > exceptions. On the other, from a reviewer's perspective, hand having too many packages to review is bad. > The problem we have in Tizen is not that we have too few > reviewers. It's that each reviewer says "there are others, they'll > review" and no one ends up doing it. > > I would have preferred that the Cc list for each contribution have no > more than 10 people. More than that, we've got the effect I mentioned. That is exactly the point. And you also need to make sure that each of thos 10 people won't get notifications for more than N (IMHO N ~ 3 and I can negotiate that) repositories. I admit I send (almost) all the gerrit messages to bit bucket (manually but still) and treat them (their presence in my INBOX) only as a reminder to go to Gerrit and take a closer look. Then I also have a script that removes me from lists of reviewers for changes of several packages I know I can't help with. Only from time to time I take a look at random one of such changes at give my comments (most of the time I hate commit messages). As I wrote in December the compromise would be: - set up lists of *notified* pople for each project separate from the "domain groups", - allow all members of the "domain groups" to excersise their permissions in all repositories of the domain. However risky the second point may look, it will help in many simple situations where one member of a team (there is currently six in mine) is sick go for holiday and the others can take his duties. -- Łukasz Stelmach Samsung R&D Institute Poland Samsung Electronics _______________________________________________ Dev mailing list [email protected]<mailto:[email protected]> https://lists.tizen.org/listinfo/dev
_______________________________________________ Dev mailing list [email protected] https://lists.tizen.org/listinfo/dev
