@dev is now CCed I didn't want to over structure the discussion with too much detail up front. I do think CTR without supporting process or boundaries can be more problematic than helpful. That could take the form of customarily waiting for reviews before commit even under a CTR regime. I think this discussion has been great so far. I would also add that CTR moves 'R' from a gating requirement per commit (which can be hard to overcome for niche areas or when volunteer resources are less available) more to RMs. will be back later with more.
> On Jul 29, 2015, at 1:36 PM, Sean Busbey <[email protected]> wrote: > > I'd also favor having this discussion on dev@. > >> On Wed, Jul 29, 2015 at 2:29 PM, Gary Helmling <[email protected]> wrote: >> >> This is already a really interesting and meaningful discussion, and is >> obviously important to the community. >> >> Is there any reason not to move this straight to the dev@ list? >> >>> On Wed, Jul 29, 2015 at 11:40 AM Todd Lipcon <[email protected]> wrote: >>> >>> I'm not very active in HBase these days, so I won't cast a non-zero vote, >>> but I'm -0 on this idea, for basically two reasons: >>> >>> 1) In my experience at a past job which used CTR, the reality was that it >>> was more like "Commit and maybe review" rather than "Commit then review". >>> It's always more fun (and often easier) to write new code than to review >>> other people's code, so if there isn't a requirement that all code gets >>> reviewed before commit, it's easy for the ratio of code written to code >>> reviewed to tend towards values significantly greater than 1:1 over time. >>> At my past job, this meant that a lot of code made it into production (it >>> was a website) that hadn't ever been reviewed, and in many cases we found >>> bugs (or other issues) that would have definitely been caught by a good >>> code reviewer. >>> >>> 2) CTR has both the advantage and disadvantage of allowing areas of code >> to >>> be evolved quickly by a single person. That could be seen as a plus, in >>> that there are some areas which tend to get ignored because we don't >> have a >>> critical mass of people reviewing patches in the area -- patches languish >>> in these areas currently. However, that could also be seen as a good >> thing >>> from a "community over code" perspective -- it's better to not have any >>> areas of code with bus-factor 1. Feeling the pain of not getting reviews >> in >>> these areas with only a single active committer encourages us to find >>> solutions - either by deprecating "niche" features (as we once did with >>> various 'contrib' projects) or by recruiting new committers who have >>> interest in maintaining that code area. Lifting review restrictions would >>> allow us to sweep bus-factor issues under the rug. >>> >>> That said, I think CTR can be a valuable tool for "move fast on new >>> experimentation" type projects -- I've participated in several feature >>> branches in HDFS where we operated on CTR on the branch. The assumption >> was >>> that, prior to merging the branch into trunk, all of the patches (or a >>> consolidated mega-patch) would be thoroughly reviewed by several other >>> committers. I found this to work very well during early development, >> since >>> I could hack on things and even commit pieces of code that had known >>> issues/TODOs. For trickier patches on the CTR branch, I still tended to >>> ping experienced contributors for their opinions and feedback before >>> committing. >>> >>> Perhaps such a hybrid policy would work well in the context of HBase >>> feature development as well? >>> >>> -Todd >>> >>> >>> On Wed, Jul 29, 2015 at 11:27 AM, Andrew Purtell <[email protected]> >>> wrote: >>> >>>> Just thought of branch merge votes. Sorry for that omission. I think it >>>> makes sense to keep those, but let's discuss that too in this context. >>>> >>>> >>>> On Wed, Jul 29, 2015 at 11:26 AM, Andrew Purtell <[email protected]> >>>> wrote: >>>> >>>>> As Greg Stein said on a thread over at general@incubator >>> ("[DISCUSSION] >>>>> Graduate Ignite from the Apache Incubator"): >>>>> >>>>> I always translate RTC as "we don't trust you, so somebody else must >>>>> approve anything you do." To me, that is a lousy basis for creating a >>>>> community. Trust and peer respect should be the basis, which implies >>> CTR. >>>>> >>>>> I happen to agree with this sentiment and would like to propose >>> switching >>>>> our consensus on commit procedure from RTC to CTR. Concurrently, I >>> would >>>>> like to propose this consensus also include the following: >>>>> - Checkins should pass precommit or the committer should explain on >> the >>>>> JIRA why precommit failures are in their best judgement not related >>>>> - The PMC should be willing to, ultimately, revoke committership >> should >>>>> trust in any individual committer be discovered to be misplaced. I >>> would >>>>> expect such a last resort to be exceedingly rare and likely never to >>>> happen >>>>> because of course long before that we would be setting correct public >>>>> examples in the first place and respectfully counseling well >>> intentioned >>>>> committers who might stray. >>>>> >>>>> Depending on how the conversation proceeds here I would like to >> include >>>>> dev@ in this conversation at the earliest opportunity as well. >>>>> >>>>> Thoughts? >>>>> >>>>> -- >>>>> Best regards, >>>>> >>>>> - Andy >>>>> >>>>> Problems worthy of attack prove their worth by hitting back. - Piet >>> Hein >>>>> (via Tom White) >>>> >>>> >>>> >>>> -- >>>> Best regards, >>>> >>>> - Andy >>>> >>>> Problems worthy of attack prove their worth by hitting back. - Piet >> Hein >>>> (via Tom White) >>> >>> >>> >>> -- >>> Todd Lipcon >>> Software Engineer, Cloudera > > > > -- > Sean
