hbase-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrew Purtell <apurt...@apache.org>
Subject Re: [DISCUSSION] Switching from RTC to CTR
Date Sun, 16 Aug 2015 18:40:33 GMT
> 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.

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. 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?)

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.


On Fri, Aug 14, 2015 at 9:00 PM, Sean Busbey <busbey@cloudera.com> 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 <andrew.purtell@gmail.com>
> 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 <stack@duboce.net> wrote:
> > >
> > > On Fri, Jul 31, 2015 at 3:15 AM, Andrew Purtell <
> > andrew.purtell@gmail.com>
> > > 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 <
> > andrew.purtell@gmail.com>
> > >> 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 <olorinbant@gmail.com
> >
> > >>>> 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
> > >>>> <andrew.purtell@gmail.com> 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 <sean.busbey@gmail.com>
> > >>> wrote:
> > >>>>>>
> > >>>>>> I'd also favor having this discussion on dev@.
> > >>>>>>
> > >>>>>>> On Wed, Jul 29, 2015 at 2:29 PM, Gary Helmling <
> > ghelmling@gmail.com
> > >>>
> > >>> 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 <todd@cloudera.com
> >
> > >>> 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
<
> > >>> apurtell@apache.org>
> > >>>>>>>> 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
<
> > >>> apurtell@apache.org>
> > >>>>>>>>> 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)

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message