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 >>
