Huge +1 to more review. Let's also setup some code guidelines for Erlang/Javascript/C/HTML so we have an authoritative list of rules to follow to ensure code consistency, and similarly, let's get some guidelines for git commits messages as well, Tim Pope's article comes to mind: http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html.
Whichever review tool we decide to use, we should use it universally. We should figure out a way to do github integration so that PR's show up in the tool, and then we update the PR with a link to the relevant review page. We also need a good way to initiate the analogue of a PR in whatever tool we use. One of the complications with the review process today, is that we're all accustomed to doing review through github PR's, but we can use github PR's for branches who exist solely on the ASF origin. I imagine this won't be an issue with Hive or Gerrit, but we should verify that we're not required to initiate code review from third party origins. On a side note, one of my personal nits with github PR's, that I hope might be resolved with one of these tools, is losing comments when the code changes. For instance, if I do a review and make 10 comments that each represent a line item that needs to be addressed, I would like to see a task list on the review indicating there are unfinished items to be addressed, basically a way to make a set of checkboxes necessary to complete the review. I find it odd that github doesn't do this, even more so by the fact that changing code to address comments will often mess with the commenting and hide the messages you posted, making it challenging to decipher whether the review items have been addressed. This is by no means a requirement to using any approach, just hoping that someone knows of a good way to approach this problem in whatever tool we use. -Russell On Wed, Jan 22, 2014 at 6:06 AM, Benoit Chesneau <bchesn...@gmail.com>wrote: > On Wed, Jan 22, 2014 at 12:47 PM, Noah Slater <nsla...@apache.org> wrote: > > > My first comment is: if we want more reviews, let's have more committers. > > > > We double our committer base in 2013, and the results look like this: > > > > https://www.ohloh.net/p/couchdb/analyses/latest/languages_summary > > > > And I see comments like this on StackOverflow: > > > > "I've recently noticed that Couch DB is back in heavy development." > > > > So I think we should continue to aggressively recruit more committers > > to the project in 2014. Excelsior! > > > > Welcoming new committers in the project is certainly a good thing but I > think it's orthogonal to the need of more review and team work. We should > find a good workflow now while we are still a limited number of committers > because It will be harder to enforce anything on a larger team. We should > settle on some guidelines now. It will also help us to attract more people > IMO. > > > > > > However, as for the way we do reviews... > > > > Infra ticket about Gerrit > > https://issues.apache.org/jira/browse/INFRA-2205 > > > > Effort seems to be mothballed. But I'm sure we could restart it if we > > were serious. > > > > However, we could use this: > > > > https://reviews.apache.org/groups/hive/ > > > > It is well integrated with Apache infrastructure already, sends mails > > to the mailing list, and so on. > > > > Happy to request an instance and we can experiment with it if we like. > > If it doesn't work out, we stop using it. No harm. Could make it > > entirely voluntary until we figure out a workflow that we all like. > > > > Should I do that? > > > > I thought gerrit was already integrated but any tool that could help us to > make the review is OK for me. How hive compares to gerrit? Could we also > plug it with github like gerrit [1] ? > > - benoît > > [1] https://gerrit.googlesource.com/plugins/github/ (and some others) > > > > > On 20 January 2014 15:30, Nick North <nort...@gmail.com> wrote: > > > On 20 January 2014 12:26, Dale Harvey <d...@arandomurl.com> wrote: > > > > > >> I fully agree, its something I mentioned at the couchdb conf in > > vancouver, > > >> a review first system encourages contributions and has multiple > benefits > > >> > > >> * At least 2 people look at the code, less likely to push silly > > mistakes > > >> * Can codify and practice review rules > > >> * Its much easier to view the current activity in the project > > >> * Can bring up points of discussion before its too late > > >> > > >> But I think the most important thing is that it removes the burden of > > >> responsibility from the committer to the project as a whole, also > hugely > > >> beneficial is that it makes it particularly obvious when a patch has > > reach > > >> a stalemate and forces someone to make the call. > > >> > > >> For reference on PouchDB every committer is trusted to push code, > nobody > > >> (including myself) pushes their own code to master, it goes in the PR > > queue > > >> and gets a +1 from any other committer (who will usually push it), > thats > > >> essentially the same process we use at mozilla and coming to think of > it > > >> any other project I have worked on, any commiter has the ability to > -1 a > > >> patch at which point they give a reason and usually some solution gets > > >> agreed on > > >> > > > > > > I like this system, with one small quibble about responsibility. I > don't > > > think it should be seen as removing the burden of responsibility from > the > > > committer to the project as a whole. It becomes easy then for everyone, > > > including the committer, to think someone else will find bugs, and > no-one > > > puts in enough effort. I'd say it is still primarily the responsibility > > of > > > the committer to ensure that code is error free, but that at least one > > > person who knows that area of the code base should sign off on it. Some > > > spreading of responsibility is good, but too much can actually lead to > a > > > decline in quality. > > > > > > > > > Nick > > > > > > > > -- > > Noah Slater > > https://twitter.com/nslater > > >
