+1 on option 1. This is standard operating practice (for all code changes, no
exceptions) at Facebook, Google, and many other companies that care about code
quality.
(The latest HBase wiki makes an exception for patches that only involve one
changed+unreviewed file, but I think that creates
I would argue that how to do the review has nothing to do with code
quality. No review board has no connection with low quality code.
The review board just provides a limited view of the diff's context.
So if the review is familiar with the diff's context and is confident
with the patch, it is
I agree on the problem of comments.
For a new reviewer, it makes the process more painful.
But, I think, the issues are not major, and we should
decide one approach or another.
Even for option 2., we can make it mandatory for the contributor
to submit a reviewboard request if the reviewer asks
On Thu, Jan 20, 2011 at 5:56 PM, Namit Jain nj...@fb.com wrote:
I agree on the problem of comments.
For a new reviewer, it makes the process more painful.
But, I think, the issues are not major, and we should
decide one approach or another.
Even for option 2., we can make it mandatory for
I was thinking about it. Let us do the following:
For big patches (a big patch is one which involves changing more than
10 files - those files can be test/code changes or even generated files),
reviewBoard is mandatory.
For smaller patches, reviewBoard is optional. However, if any reviewer
asks
On Jan 20, 2011, at 10:53 PM, Namit Jain wrote:
As good practice, also copy important review comments in the jira, so that
they are searchable and give enough context to a new contributor.
The old review board instance did this automatically. Todd has an INFRA JIRA
open to get this for
+1 for option 2.
In general, we as a community should be nice to all contributors, and
should avoid doing things that make contributors not comfortable, even
that requires some work from committers. Sometimes it is especially
true for new contributors, like we need to be more patience for new
The system that we have in place right now places all of the burden on the
reviewer. If you want to look at a patch you have to download it, apply it
to a clean workspace, view it using the diff viewer of your choice, and then
copy your comments back to JIRA along with line numbers and code
