> - What constitutes a valid review? I think the community will get to decide, but what I've seen in other projects is there has to be a +1 from a committer on the project. The +1 can be a comment in JIRA, clicking the ship it button in Review Board, or any other method where the +1 has been recorded in ASF infrastructure. For something like Hadoop, this has been fairly formal with a +1 needing to happen in JIRA before something can be committed. I know Parquet uses pull requests, but I'm not sure if they allow +1's to just be comments in github or if they need to also be provided in the JIRA.
My general belief is that either JIRA or a review tool is fine. Generally, a formal vote isn't required for a review and would only be needed if a committer has vetoed a change and consensus can't be reached through the normal review back and forth. > - What is the best tooling/process around this to actually conduct/document > the review? There are two things you typically want, a standard for how code/patches are submitted. Going down to the what git commands to run to format the patch is very useful as each project is a little different. If we're able to use pull requests, then that's less critical as the pull request will handle those details. The second is where to do the review comments themselves. I really like tools like Review Board or GitHub's pull request review interface. Having a web based way of providing per-line comments makes things flow much more easily than comments in JIRA or mailing list discussions. > - What are some key things a reviewer should be checking for and helping to > enforce? What are some out of bounds things? There are two things that the reviewer should be focusing on. How will this change improve the project and how can I help mentor the contributor to generally improve the quality of their contributions. That means that anything is in scope, but the focus of the review should always be on how to guide the contributor to make the best improvement to the project. If we want to adopt formal code style guidelines, then we should document those and ideally have automated ways of checking those (checkstyle, etc.) so that contributors can verify for themselves rather than have to wait for review feedback. Generally a veto on a code change requires a "valid technical reason". I think the community has to decide what falls under that definition. In my head it would include things like: introduces a security hole, introduces a major performance regression, or violates or compatibility guidelines for the release that it's targeting. -Joey On Wed, Dec 10, 2014 at 6:18 PM, Joe Witt <[email protected]> wrote: > NiFi grew up in a CT[R] environment where the review was very rare and so > really we ended up with CTDTFSOR - Commit then deploy then feel sense of > regret. > > So the bad side: We ended up with a lot of avoidable defects where by > avoidable I mean they could have likely been detected in a decent review > effort. > > The good side: We were fast. Very fast. New features got out quick and if > something was funky we fixed it very fast. For the vast majority of > mistakes the problem was highly isolated (thanks to our base design > construct of FBP) and rarely caused extremely stressful days. > > On balance though and given the benefits it has to community growth I > personally am very supportive of us adopting RTC immediately provided we > can sort out some key questions: > > - What constitutes a valid review? > This page http://www.apache.org/foundation/glossary.html#ReviewThenCommit > suggests that a review is a consensus approval. I am very concerned by > that definition if we're talking every commit means we need a vote. For > the vast majority of commits this just seems to onerous and too time > consuming and frankly to me takes some of the fun out of developing. If > for us a review is simply that a 'committer' has reviewed the code then I > am perfectly happy with this and I am thinking this is in-line with the > model Benson described for his dayjob "'make branch, submit pr, get review, > merge'" > > - What is the best tooling/process around this to actually conduct/document > the review? > I think > https://cwiki.apache.org/confluence/display/KAFKA/Patch+submission+and+review#Patchsubmissionandreview-Simplecontributorworkflow > does a good job of describing the mechanics of a review/submission > process. And I'd just assume a patch is as good as a pull request. I > think even as a good starting point having the JIRA ticket commented on by > the reviewer would be good. > > - What are some key things a reviewer should be checking for and helping to > enforce? What are some out of bounds things? > > > > This seems like a really important discussion and something that we should > center on sooner than later. There are ways to nuance this such as RTC for > the core and CTR for extensions and so on but I think that just leads to > more confusion. We should likely pick our poison. I see strong benefits in > both directions and so I can deal either way personally. I see a couple > strong arguments here in this thread already for RTC . > > Anyone willing to argue for CTR? > > Thanks > Joe > > > On Wed, Dec 10, 2014 at 8:21 PM, Benson Margulies <[email protected]> > wrote: > >> It seems to me, and this is purely opinion, that RTC has a lot to be said >> for a new project, in terms of getting more people involved, more eyeballs, >> and more of a community feeling. My dayjob workflow is 'make branch, submit >> pr, get review, merge' and that seems pretty applicable. However, I am not >> wearing any magic mentor hat-o-authority. >> >> On Wed, Dec 10, 2014 at 2:23 PM, Billie Rinaldi <[email protected]> wrote: >> >> > On Tue, Dec 9, 2014 at 8:07 AM, Joe Witt <[email protected]> wrote: >> > >> > > Benson - thanks for the headsup on the maven plugin. Seems like using >> > them >> > > to their fullest capability and then manually merging to master is >> > > perfectly fine to me. >> > > >> > > Billie >> > > As for CTR i don't think i have a good enough appreciation for the >> > > process/value proposition here. Curious of other folks views. >> > > >> > >> > Both CTR and RTC have their own advantages, so either would be fine ( >> > http://www.apache.org/foundation/glossary.html#CommitThenReview). In >> > retrospect I see my question could be viewed as leaning towards CTR, but >> it >> > was just based on an observation that people already seemed to be making >> > commits without review. >> > >> > >> > > >> > > It seems reasonable to keep the feature branch relaxed and in that >> sense >> > > what Gilman did today seems fine. We can get bettter at those >> judgement >> > > calls and documenting the criteria as we go along. >> > > >> > > Thanks >> > > Joe >> > > >> > > >> > > >> > > On Tue, Dec 9, 2014 at 11:00 AM, Benson Margulies < >> [email protected] >> > > >> > > wrote: >> > > >> > > > PR's are certainly convenient. There's no much difference, for a >> > > committer, >> > > > between pushing a branch to the official repo and pushing a branch to >> > > some >> > > > personal repo on github. The same integration workflow can be used >> > either >> > > > way to close out the PR upon merge. >> > > > >> > > > However, in a CTR project, it seems perhaps excessive to _require_ >> > > feature >> > > > branches for small fixes as opposed to just committing them directly >> to >> > > > develop. At day job we do mostly do branch-per-jira, but some people >> > > might >> > > > find that onerous. >> > > > >> > > > Pushing to the official repo leaves more history that someone might >> > find >> > > > interesting some day. Also more clutter; some people are very >> concerned >> > > > about repacking before merging to develop. >> > > > >> > > > Another issue with gitflow is the master branch. The master branch is >> > > > supposed to get merged to for releases. The maven-release-plugin >> won't >> > do >> > > > that, and the jgitflow plugin is unsafe. So one option is to 'use >> > > gitflow' >> > > > but not bother with the master versus develop distinction, the other >> is >> > > to >> > > > do manual merges to master at release points. >> > > > >> > > > >> > > > On Tue, Dec 9, 2014 at 10:22 AM, Joe Witt <[email protected]> >> wrote: >> > > > >> > > > > Hello >> > > > > >> > > > > So a question on gitflow given that commits are now underway. When >> > > > working >> > > > > on a feature in a local repo which is a branch off the develop >> > > > branch...do >> > > > > you push the feature branch to the central repo? This then can be >> > > merged >> > > > > by someone else as a sort of code review/pull process? >> > > > > >> > > > > Thanks >> > > > > Joe >> > > > > >> > > > > On Mon, Dec 8, 2014 at 11:40 PM, Joe Witt <[email protected]> >> > wrote: >> > > > > >> > > > > > All, >> > > > > > >> > > > > > Now that we have our code up it is important to establish a >> process >> > > > > around >> > > > > > git in particular. A general consensus in the thread appears to >> be >> > > > that >> > > > > > gitflow workflow is a reasonable option. >> > > > > > >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> https://www.atlassian.com/git/tutorials/comparing-workflows/gitflow-workflow >> > > > > > >> > > > > > To that end I've added a develop branch off of master from which >> > > > features >> > > > > > can be built. As we converge toward a release then we'll >> > > > > address/introduce >> > > > > > some of the other aspects of gitflow. >> > > > > > >> > > > > > Please discuss/comment if there are views that we should be >> taking >> > > > > another >> > > > > > path. >> > > > > > >> > > > > > Thanks >> > > > > > Joe >> > > > > > >> > > > > > On Sat, Nov 29, 2014 at 8:16 AM, Benson Margulies < >> > > > [email protected] >> > > > > > >> > > > > > wrote: >> > > > > > >> > > > > >> On Sat, Nov 29, 2014 at 3:45 AM, Sergio Fernández < >> > > [email protected]> >> > > > > >> wrote: >> > > > > >> > Hi Adam, >> > > > > >> > >> > > > > >> > one remarks about this: >> > > > > >> > >> > > > > >> > On 28/11/14 18:07, Adam Taft wrote: >> > > > > >> >> >> > > > > >> >> Knowing how we work today, if it were me, I would suggest >> using >> > > the >> > > > > >> above >> > > > > >> >> workflow combined with the "forking workflow" to guard access >> > to >> > > > the >> > > > > >> >> production release (master) branches. A very small subset of >> > the >> > > > > >> >> incubator's commiters should have the ability to merge the >> > > > "develop" >> > > > > >> >> branch >> > > > > >> >> down to a master "release" branch. >> > > > > >> > >> > > > > >> > >> > > > > >> > Suck workflow is not in place in ASF. On the one hand, the >> > current >> > > > git >> > > > > >> > infrastructure does not provide such branches' management, >> like >> > > > > >> > bitbucket/stash do for instance. On the other hand, and more >> > > > > important, >> > > > > >> a >> > > > > >> > project is not hierarchical organization, but a a meritocratic >> > > one. >> > > > > >> > >> > > > > >> > I recommend you this blog post in case you want to read a bit >> > > more: >> > > > > >> > >> http://communityovercode.com/2012/05/meritocracy-and-hierarchy/ >> > > > > >> > >> > > > > >> > If someone has permissions to do (i.e., he is a committer), he >> > can >> > > > do >> > > > > >> it, >> > > > > >> > simple The tool provide you instruments to revert those >> changes >> > in >> > > > > case >> > > > > >> on >> > > > > >> > involuntary errors. >> > > > > >> > >> > > > > >> >> It would be ideal to have someone who >> > > > > >> >> is NOT performing the majority of changes on the "develop" >> > branch >> > > > > take >> > > > > >> >> this >> > > > > >> >> role to coordinate releases, ensure minimal coding standards, >> > run >> > > > > >> through >> > > > > >> >> unit and integration tests, before signing off on the release >> > and >> > > > > >> issuing >> > > > > >> >> the release artifacts. >> > > > > >> >> > > > > >> You seem to be imagining an individual with a job which is >> shared >> > in >> > > > > >> by the community. In healthy communities, a release happens when >> > > > > >> there's a consensus to have a release. There is no person who >> > > 'ensures >> > > > > >> minimal coding standards', that's everyone watching commits. >> > There's >> > > > > >> no one 'running unit and integration tests' because (a) every >> > > > > >> committer does this before every commit, (b) Jenkins does it, >> (c) >> > > the >> > > > > >> release process does it. (d) there's no signing off on a >> release. >> > > The >> > > > > >> RM puts it up for a vote, and PMC members vote. >> > > > > >> >> > > > > >> >> > > > > >> > >> > > > > >> > >> > > > > >> > Release comes after that. The release manager is responsible >> on >> > > > > >> creating a >> > > > > >> > proper release, which must include a code release and should >> > > include >> > > > > >> > binaries too. Each artifact release must be signed. >> Demonstrate >> > > your >> > > > > >> ability >> > > > > >> > as a project to produce releases is one of the goals of the >> > > > > incubation. >> > > > > >> But >> > > > > >> > we are not yet there, step by step. >> > > > > >> > >> > > > > >> > Hope that helps. >> > > > > >> > >> > > > > >> > Cheers, >> > > > > >> > >> > > > > >> > >> > > > > >> > -- >> > > > > >> > Sergio Fernández >> > > > > >> > Partner Technology Manager >> > > > > >> > Redlink GmbH >> > > > > >> > m: +43 660 2747 925 >> > > > > >> > e: [email protected] >> > > > > >> > w: http://redlink.co >> > > > > >> >> > > > > > >> > > > > > >> > > > > >> > > > >> > > >> > >> -- Joey Echeverria
