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 the contributor > to submit a reviewboard request if the reviewer asks for it. > > Can others vote please ? > > > > Thanks, > -namit > > > > > On 1/20/11 2:25 PM, "yongqiang he" <heyongqiang...@gmail.com> wrote: > >>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 his call to decide the review process. And also >>even with the review board, the reviewer sometimes still need to open >>his workspace to look for more contexts, like the references and the >>caller of a function etc. >> >>The reason that option 2 looks good to me are the rule here is just >>assuming people in this community acting nice to each other. The >>committer/reviewer creates a review board for the contributor because >>he is nice to the contributor, right? And the contributor should also >>create review board for following patches in the same issue because he >>knows the reviewer needs a review board to review his patch, and he >>should be nice to the reviewer by doing that himself. >> >>Another thing came up from an offline discussion is that are the >>comments on review board coming back to the jira? If no, that means >>the comments are not searchable, and the later followers, who want to >>get a whole understanding of the problem/solutions/pitfalls, need to >>open the patch/review board page to find all the comments from >>reviewers. >> >>And I want to clear that I am not agains review board. I would suggest >>let us move on with what each one feels comfortable and more >>productable, and avoid enforcing some rules on this. >> >>thanks >>-yongqiang >>On Thu, Jan 20, 2011 at 12:16 PM, Todd Lipcon <t...@cloudera.com> wrote: >>> On Thu, Jan 20, 2011 at 12:12 PM, John Sichi <jsi...@fb.com> wrote: >>> >>>> +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 a bad incentive for >>>>people >>>> to squash stuff into one file, e.g. via inner class, instead of >>>>properly >>>> refactoring in cases where it is called for.) >>>> >>> >>> huh, I had no idea that was the case for HBase :) >>> >>> We generally follow the policy that you can Commit-Then-Review for >>>obviously >>> trivial things (eg a doc update or spelling fix or something of that >>>sort) >>> >>> >>>> >>>> To better facilitate this policy, we should make sure that the >>>>workflow is >>>> as smooth as possible. >>>> >>>> (1) Make usage of postreview much more pominent in the HowToContribute >>>> docs, and do everything possible to raise awareness about its >>>>existence. I >>>> think we should also configure our repositories and check in a wrapper >>>> script in order to make post-review usage a no-brainer: ideally, we >>>>would >>>> be able to just give it a HIVE-xyz JIRA number, and the script would >>>>wget >>>> the necessary information and include that in the postreview >>>>submission. >>>> There should be very few cases where anyone needs to go through the >>>>Review >>>> Board UI. See this section and following for more: >>>> >>>>http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/#repo >>>>sitory-configuration >>>> >>>> (2) See if we can configure JIRA to require a review board link as >>>>part of >>>> submitting a patch. This makes the system self-enforcing. Ideally, >>>>JIRA >>>> would automatically pull the patch from review board so that the >>>>contributor >>>> does not have to do a separate patch-generation + upload. >>>> >>>> (3) Eliminate all generated code from the codebase; this causes a lot >>>>of >>>> extra friction. Now that we have moved to an official thrift release, >>>>this >>>> should be easier. >>>> >>>> If we do all of these, I think the net result will actually be a better >>>> experience for contributors relative to what we have today. >>>> >>>> JVS >>>> >>>> On Jan 19, 2011, at 9:05 PM, Carl Steinbach wrote: >>>> >>>> > 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 >>>> fragments >>>> > in order to provide context for the author. If there's more than one >>>> > reviewer, then everyone repeats these steps individually. From this >>>> > perspective I think using ReviewBoard is a clear win. It eliminates >>>>the >>>> > setup steps that are currently incumbent on the reviewer and >>>>consequently >>>> > encourages more people to participate in the review process, which I >>>> think >>>> > will result in higher quality code in the end. >>>> > >>>> > I think that the additional burden that ReviewBoard places on the >>>> > contributor is very small (especially when compared to the effort >>>> invested >>>> > in producing the patch in the first place) and can be mitigated by >>>>using >>>> > tools like post-review ( >>>> > http://www.reviewboard.org/docs/manual/dev/users/tools/post-review/). >>>> > >>>> > I'm +1 for option (1), meaning that I think people should be >>>>required to >>>> > post a review request (or update an existing request) for every patch >>>> that >>>> > they submit for review on JIRA. I also think excluding small patches >>>> from >>>> > this requirement is a bad idea because rational people can disagree >>>>about >>>> > what qualifies as a small patch and what does not, and I'd like >>>>people to >>>> > make ReviewBoard a habit instead of something that they use >>>>occasionally. >>>> I >>>> > think that Yongqiang's point about scaring away new contributors with >>>> lots >>>> > of requirements is valid, and I'm more that willing to post a review >>>> request >>>> > for a first (or second) time contributor, but in general it's >>>>important >>>> for >>>> > the contributor to create the request since only the creator can >>>>update >>>> it. >>>> > >>>> > Thanks. >>>> > >>>> > Carl >>>> > >>>> > >>>> > >>>> > >>>> > >>>> > On Wed, Jan 19, 2011 at 6:48 PM, yongqiang he >>>><heyongqiang...@gmail.com >>>> >wrote: >>>> > >>>> >> +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 >>>> >> people. It seems a free style and contribution focused environment >>>> >> would be better to encourage people to do more contributions of >>>> >> different kinds. >>>> >> >>>> >> thanks >>>> >> -yongqiang >>>> >> On Wed, Jan 19, 2011 at 6:37 PM, Namit Jain <nj...@fb.com> wrote: >>>> >>> >>>> >>> >>>> >>> >>>> >>> It would be good to have a policy for submitting a new patch for >>>> review. >>>> >>> If the patch is small, usually it is pretty easy to review.But, if >>>>it >>>> >> large, >>>> >>> a GUI like reviewboard (https://reviews.apache.org) makes it easy. >>>> >>> >>>> >>> So, going forward, I would like to propose either of the following. >>>> >>> >>>> >>> 1. All patches must go through reviewboard >>>> >>> 2. If a contributor/reviewer creates a reviewboard request, >>>> >>> all subsequent review requests should go through the >>>>reviewboard. >>>> >>> >>>> >>> >>>> >>> I would personally vote for 2., since for small patches, we don¹t >>>> really >>>> >> need a >>>> >>> reviewboard. >>>> >>> >>>> >>> But, please vote, and based on that, we can come up with a policy. >>>> >>> Let us know, if you think of some other option. >>>> >>> >>>> >>> Thanks, >>>> >>> -namit >>>> >>> >>>> >>> >>>> >> >>>> >>>> >>> >>> >>> -- >>> Todd Lipcon >>> Software Engineer, Cloudera >>> > >
I like #2. Trivial patches are just that. Bigger stuff almost needs review board. For thousand line patches, if someone comments simply finding what they are referring to can be a pain. Then in the next revision checking that all your comments were addressed are pain.
