On Sun, Aug 16, 2015 at 11:40 AM, Andrew Purtell <[email protected]> wrote:
> > As Stack mentioned, having the communal norm of reviews means that folks > are more likely to see more of the code. > > I don't get it, because that norm can exist equally under CTR as RTC. > > How? If strictly CTR, yes, but I've been reading CTR in this discussion as CTOR (Commit, Then, Optionally Review). Correct me if I have it wrong. > Let me repeat a question I had earlier that nobody has responded to: What > would be the practical difference/outcome of a committer immediately > checking in something today without giving time for review versus doing so > if we said "CTR" or "CTR after one week" *AND* "good committer practice is > to get a code review and +1 before commit"? Practically, I mean. Probably none -- former would be 'abnormal' so would likely trigger peer interjection while the latter would be the norm and more likely to be let slide. In both > cases we'd revert it and have a discussion. Would the lack of attention to > good practice be more actionable by the PMC because in the first case it is > a rule violation and in the latter it's just inattention to community > norms. Is that it? We need rules? (And is that "just because", or is there > really a lack of trust?) > > Yes. We need a few rules and a bit of process. Having a simple set makes distributed dev by a bunch of peers run smoother. Above in this thread are a few arguments on how RTC does not have to be interpreted as lack of trust. > Anyway, I like how in this discussion members of the PMC have expressed > some room for committers to make minor commits on their good judgement > without _necessarily_ getting a review first in order to make progress. My > motivation here is we are a community of busy people and some things just > can't get the level of interest as others. As an RM, things like build > fixes to unblock release candidates of older branches come to mind. I also > like that we have plenty of room for experimental work on branches with > scrutiny coming later at branch commit time with a very reasonable bar of 3 > +1s inviting necessary scrutiny. I still think a policy of "one week for > review, then proceed as CTR" would be useful for a variety of reasons but > don't see clear support for that here. After I collect a few instances of > recent anecdotal evidence supporting it I may revive this discussion. > > Ok. St.Ack > > On Fri, Aug 14, 2015 at 9:00 PM, Sean Busbey <[email protected]> wrote: > > > My apologies if this thread has run its course and I'm showing up late to > > rehash things. > > > > Here's a short list of the reasons I like RTC: > > > > 1) Number One With A Bullet: It puts committers and > > non-committer-contributors on closer to equal footing. If I'm > participating > > in the project and I haven't been blessed with a commit bit, what am I > > supposed to do after my week of having my patch sit around? > > > > 2) Community interaction. As Stack mentioned, having the communal norm of > > reviews means that folks are more likely to see more of the code. > > > > 3) Everyone has a bad day. I totally identify with committership being a > > sign of "I trust you" to project participants. But everyone has one of > > those days where you're in a rush either because of work or life. Having > > even a cursory additional set of eyes on things markedly increases the > > quality of the overall code base over a long enough time line (at least > in > > my experience contributing to open source projects). So for me, the trust > > is largely "to follow the rules" and "to provide feedback in reviews". > > > > If we end up with some part of the code that isn't getting reviews, I'd > > rather the PMC fix that problem instead of backslide on the three points > > above. > > > > That we don't have this problem right now is wonderful. I have been in / > am > > currently in some projects where the community is very near end-of-life > by > > the ASF's definition of 3 folks to approve a release. My observation of > > those communities has the CTR ones much more a loose collection of people > > scratching their own itch. When an ASF project gets to that point, what > the > > advantage over just going to the attic and keeping independent forks for > > those few remaining folks? > > > > I kind of view this like the ASF policy on only distributing PMC approved > > releases. The advice from the foundation for folks who don't like the > > limitation of waiting for a release is "make more releases." Similarly, I > > think the problem of reviewer bandwidth is solved with "make more > > committers." > > > > I don't want to hijack this thread, but maybe we could have another one > > about expectations for committership and ways the PMC could help get more > > folks on the path? > > > > > > On Sun, Aug 2, 2015 at 1:59 PM, Andrew Purtell <[email protected] > > > > wrote: > > > > > Agreed, committers should be spending more time reviewing others' work. > > We > > > can ask. It may happen. It may work for a short while. It may not. > Shrug. > > > People will do what they want. > > > > > > I'm looking to make one substantial change that will allow committers > to > > > make progress even if there's nobody else around or interested for one > > > week. It happens sometimes. I've already talked about my concerns on > > > assuming a certain level of available volunteer bandwidth. Let me just > > say > > > that things are great now, it's fantastic. > > > > > > We are pretty much on the honor system already. I don't buy the > argument > > > we can't trust that CTR or CTR after one week can work because > > committers, > > > even if asked to customarily get a review before commit, may decide to > > > check in unreviewed untested destabilizing changes. At least, In that > > case > > > I'd argue we have a different problem. If you go back to my original > > mail, > > > I do say we shouldn't undertake any change if we as PMC are unwilling > to > > > revoke the commit bit from committers who sadly demonstrate themselves > > > unworthy of trust through checkins of toxic waste without review. Merit > > > _can_ be un-earned. Commits can be reverted. Just because something is > > > checked in does not mean it will go out in a release. We could put a > > > nightly suite of regression tests in place. Both proposals I have made, > > > especially the latter, are not a binary release of any attempt at > quality > > > control. We would have all of the consensus expectations of good > > committer > > > behavior still in place. > > > > > > Let's assume someone checks in something without getting a review > today. > > > What would happen? Someone else would revert it and we'd have a > > discussion. > > > If we were operating under CTR-after-one-week (or plain CTR) but with > > > documented expectations that someone get a review first, what changes, > in > > > terms of quality control and project discipline? Maybe the difference > is > > we > > > could more easily justify revoking commit privileges? Maybe. I think > > every > > > discussion like that would be strictly case by case though, a > > conversation > > > between that person and the PMC, justification for revoking committer > > > status wouldn't be rule based. > > > > > > What definitely would change is my well tested good faith change > waiting > > > for review after one week could go in no matter who's on vacation or > > > whatever. Not saying this is a problem for me today. Like I said, > things > > > are great around here today. Awesome. > > > > > > > > > > On Aug 2, 2015, at 10:52 AM, Stack <[email protected]> wrote: > > > > > > > > On Fri, Jul 31, 2015 at 3:15 AM, Andrew Purtell < > > > [email protected]> > > > > wrote: > > > > > > > >> I appreciate very much the earlier feedback about switching from RTC > > to > > > >> CTR. It helped me think about the essential thing I was after. > > > >> > > > >> I'm thinking of making a formal proposal to adopt this, with a VOTE: > > > >> > > > >>> After posting a patch to JIRA, after one week if there is no review > > or > > > >> veto, a committer can commit their own work. > > > >> > > > >> (On a bit of a lag...) > > > > > > > > What is the problem we are trying to solve? Blocked committers unable > > to > > > > get a review? > > > > > > > > Any commit can destabilize. Commits without peer review will > > destabilize > > > > more than those that have been reviewed. As a community, I'd think > that > > > > anything we can do to act against entropy in the system would be > > primary > > > > above all other considerations especially given our project a mature > > > > datastore. > > > > > > > > In my own case, it seems to take me tens of patch postings to arrive > at > > > > something a fellow reviewer thinks worthy of commit when working > with a > > > > peer on an issue of substance. Often the final patch is radically > > > different > > > > from first posting. If auto-commit, the first version would just go > in. > > > > Unless review, I'd be 'done'. If review, each iteration would be in > the > > > > code base? Churn would go up. Commits would be more and smaller. Our > > > > project would be made of less coherent 'change sets'. > > > > > > > > I'd be with LarsH suggestion that committers should be allowed go > ahead > > > > with one-liners or more substantial, non-controversial changes in > site > > or > > > > doc but beyond this, I'd be against auto-commit. > > > > > > > >> > > > >> > > > >> Also, looking at https://hbase.apache.org/book.html#_decisions, I > > don't > > > >> think the "patch +1 policy" should remain because the trial OWNERS > > > concept > > > >> hasn't worked out, IMHO. The OWNERS concept requires a set of > > constantly > > > >> present and engaged owners, a resource demand that's hard to square > > with > > > >> the volunteer nature of our community. The amount of time any > > committer > > > or > > > >> PMC member has on this project is highly variable day to day and > week > > to > > > >> week. I'm also thinking of calling a VOTE to significantly revise > or > > > >> strike this section. > > > > Yeah. OWNERS project failed. Its ghost is in effect in that we each > go > > to > > > > the domain expert when its clear (e.g. Matteo or Stephen on pv2). > > > > > > > > > > > >> Both of these things have a common root: Volunteer time is a very > > > precious > > > >> commodity. Our community's supply of volunteer time fluctuates. I > > would > > > >> like to see committers be able to make progress with their own work > > > even in > > > >> periods when volunteer time is in very short supply, or when they > are > > > >> working on niche concerns that simply do not draw sufficient > interest > > > from > > > >> other committers. (This is different from work that people think > isn't > > > >> appropriate - in that case ignoring it so it will go away would no > > > longer > > > >> be an option, a veto would be required if you want to stop > something.) > > > > Committers should be spending more time reviewing their peers work > (and > > > > just as importantly, the work of contributors)? > > > > St.Ack > > > > > > > > > > > > > > > >> On Wed, Jul 29, 2015 at 3:56 PM, Andrew Purtell < > > > [email protected]> > > > >> wrote: > > > >> > > > >>> Had this thought after getting back on the road. As an alternative > to > > > any > > > >>> sweeping change we could do one incremental but very significant > > thing > > > >> that > > > >>> acknowledges our status as trusted and busy peers: After posting a > > > patch > > > >> to > > > >>> JIRA, after one week if there is no review or veto, a committer can > > > >> commit > > > >>> their own work. > > > >>> > > > >>> > > > >>>>> On Jul 29, 2015, at 2:20 PM, Mikhail Antonov < > [email protected] > > > > > > >>>> wrote: > > > >>>> > > > >>>> Just curious, I assume if this change is made, would it only apply > > to > > > >>>> master branch? > > > >>>> > > > >>>> -Mikhail > > > >>>> > > > >>>> On Wed, Jul 29, 2015 at 2:09 PM, Andrew Purtell > > > >>>> <[email protected]> wrote: > > > >>>>> @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 > > > >>>> > > > >>>> > > > >>>> > > > >>>> -- > > > >>>> Thanks, > > > >>>> Michael Antonov > > > >> > > > > > > > > > > > -- > > Sean > > > > > > -- > Best regards, > > - Andy > > Problems worthy of attack prove their worth by hitting back. - Piet Hein > (via Tom White) >
