Great - I will spend some time today putting something together that we can iterate over.
On Thu, Jan 7, 2016 at 2:29 PM, Sumit Gupta <sumit.gu...@hortonworks.com> wrote: > +1. I’m good with this. > > > On 1/7/16, 12:42 PM, "larry mccay" <larry.mc...@gmail.com> wrote: > > >Okay - let's bring this to a close - with a slight clarification on the > >[REVIEW] emails... > > > >As long as we are all in agreement on the following, I will attempt to > >document it as our contribution policy (with a follow up email for the > >draft): > > > >* Knox has a CTR policy for committers > >* For any patches that make fundamental architectural or security related > >changes - committers should solicit feedback on the design > >* For any patches that are extensions to existing patterns for features - > >such as adding new service API support, the committer may commit freely - > >given sufficient tests and documentation (to the committer's discretion) > >* For any patches that are simple bug fixes, the committer may commit > >freely > > > >* In order to facilitate reviews from other community members post commit, > >we will use an email with a [REVIEW] tag to indicate that comments are > >being provided for a merged change and that it needs attention. > > > >There is nothing saying that we can't revisit the CTR policy in the future > >and we will continue to leverage the contributions of the community at > >large. If at any point, any of our development process seems to be barrier > >for contributors we will need to be open to change. > > > >@Sumit - note that the above does not describe a [REVIEW] email for every > >feature. > > > >Features continue to be described in JIRA and designs in attached > >documents > >or wiki. > >Reviews should also be conducted in JIRA. > >Reviews for something already committed and the JIRA is closed can have a > >[REVIEW] email as a flag to the committers to give attention to the > >feedback. > > > >If everyone is good with the above, I will draw up a draft. > > > >On Thu, Jan 7, 2016 at 11:42 AM, Sumit Gupta <sumit.gu...@hortonworks.com > > > >wrote: > > > >> Sorry for coming late to the discussion. Like Kevin, I seem to be > >> constantly missing email from the dev list (the email server doesn¹t > >>like > >> me). > >> > >> I¹m not sure how having a CTR policy for most things and then a > >>selective > >> RTC policy helps drive community involvement. It seems hard to > >>implement. > >> If anything, I can see more [DISCUSS] threads and like Larry suggested a > >> [REVIEW] email for all features. This would help awareness as well as be > >> useful in obtaining critical feedback. I would have preferred a JIRA > >> mechanism for asking for a review since we get JIRA emails on the dev > >>list > >> anyway, but I can see a review tag or comment getting lost in swarm of > >> JIRA email. > >> > >> In short, I would prefer a single straightforward policy of CTR and > >>other > >> means of raising awareness and soliciting input. > >> > >> > >> On 1/7/16, 8:40 AM, "larry mccay" <lmc...@apache.org> wrote: > >> > >> >I am not looking to change our policy at this point but instead to > >>define > >> >the expectations of CTR in the Knox community - largely based on what > >>has > >> >been done all along unofficially. The [REVIEW] email to dev@ seems a > >> >decent > >> >way to raise attention to a review for committed code. > >> > > >> >On Thu, Jan 7, 2016 at 12:15 AM, Kevin Minder > >> ><kevin.min...@hortonworks.com> > >> >wrote: > >> > > >> >> So in short: Bugs==CTR, Features==RTC? > >> >> > >> > > >> >I don't know that Features==RTC is required for extensions of existing > >> >patterns. > >> >For instance, adding support for a new service shouldn't require RTC. > >> > > >> >This does bring something to mind though. If there is complicated > >>custom > >> >HA > >> >related dispatch code then that should probably be documented or > >> >communicated to the dev@ and it would be up to the committer to > >>determine > >> >whether they would like to ask for a review. > >> > > >> >Understanding the HA dispatch algorithms for services by the dev > >>community > >> >or at least the ability to get to an understanding through readily > >> >available documentation. > >> > > >> > > >> >> > >> >> From: larry mccay <lmc...@apache.org<mailto:lmc...@apache.org>> > >> >> Date: Wednesday, January 6, 2016 at 11:06 PM > >> >> To: Kevin Minder <kevin.min...@hortonworks.com<mailto: > >> >> kevin.min...@hortonworks.com>> > >> >> Cc: "dev@knox.apache.org<mailto:dev@knox.apache.org>" > >> >><dev@knox.apache.org > >> >> <mailto:dev@knox.apache.org>> > >> >> Subject: Re: [DISCUSS] CTR, Requiring Patch Attachments and Cool Off > >> >>Period > >> >> > >> >> Hi Lars - > >> >> > >> >> Thanks for bumping this thread again - we do need to bring it to a > >> >>close. > >> >> > >> >> I certainly agree with Kevin on CTR as the current and well working > >> >>policy > >> >> for this project. > >> >> > >> >> @Kevin - I believe that we need to also consider the cool off period > >> >>yet. > >> >> I do not believe that external contributions are being blocked by the > >> >>lack > >> >> of a cool off period. > >> >> > >> >> We have numerous externally contributed features, enhancements to > >> >>existing > >> >> features and bug fixes. > >> >> > >> >> Sorry to hear that you have had bad experiences with other projects > >> >> ignoring reviews. > >> >> Let me make sure that I understand what you describe... > >> >> > >> >> I think that you are saying that you provided review comments and > >> >> suggestions to someone's patch for some apache project and that the > >> >>review > >> >> comments were never responded to and the patch was committed without > >> >> acknowledgement. Maybe you provided a review for code that was > >>already > >> >> committed and it was ignored. > >> >> > >> >> I can see the review being easier to miss/ignore when the code and > >>JIRA > >> >>is > >> >> already committed and closed. > >> >> > >> >> My proposal is that we document the following (most of which is from > >>my > >> >> original email): > >> >> > >> >> * Changes, that are aligned with existing design patterns and Knox > >> >> architecture, that are either incremental enhancements/features or > >>bug > >> >> fixes can be purely CTR > >> >> * Changes that necessitate architectural changes, have significant > >> >> security implications, or are generally large should be reviewed. > >>This > >> >>is > >> >> to facilitate communication of such changes as well as to have > >>another > >> >>set > >> >> of eyes for the code. > >> >> * Architectural changes and completely new features should be > >> >>communicated > >> >> via the dev@ list and possibly within a wiki page for design > >> >> discussion/communication. > >> >> * Review feedback for patches that have already been committed and > >>JIRAs > >> >> closed/resolved should also be sent to the dev@ list with a [REVIEW] > >> tag > >> >> in the subject. Committers will need to assess the points raised and > >> >> respond. Optionally, based on discussion the JIRA can be reopened, > >> >>possibly > >> >> commit reverted or new JIRAs filed to address the reviewer's > >>feedback. > >> >> > >> >> I think that this will fully communicate our development process and > >> >> provide rules of engagement for post commit reviews. > >> >> > >> >> Let me know your thoughts on this plan. > >> >> > >> >> thanks, > >> >> > >> >> --larry > >> >> > >> >> On Wed, Jan 6, 2016 at 10:31 PM, Kevin Minder < > >> >> kevin.min...@hortonworks.com<mailto:kevin.min...@hortonworks.com>> > >> >>wrote: > >> >> First apologies. Seems my mail server has been randomly eating > >>Apache > >> >> mail. I didn¹t see the original mail here. > >> >> > >> >> First point, attached patches. I used to attach patches to all my > >>jira. > >> >> Then I realized that a) the jira had links with diffs for the lazy > >>and > >> >>³git > >> >> format-patch <commit-sha>² will generate the patch for those that > >>want > >> >>to > >> >> use their favorite tool on a local patch file. At that point > >>requiring > >> >> patch generation from committers for a CTR project just seemed like > >> >>process > >> >> for the sake of process. This being said we could do a better job of > >> >> documenting this for those unfamiliar with git if Larry hasn¹t > >>already > >> >> taken care of that. > >> >> > >> >> Second point, CTR. We have been CRT from the beginning and this > >>model > >> >> certainly made sense given the rapid pace of development. Things > >>have > >> >> certainly slowed down recently and we can continue the discussion. > >> >> However, the core of the question you are really asking is would more > >> >> people contribute if we were RTC. I don¹t think so. We fairly bend > >>of > >> >> backwards to embrace traffic on user@knox and dev@knox. I think > that > >> is > >> >> far more significant than a CTR vs RTC policy. > >> >> > >> >> > >> >> > >> >> On 1/6/16, 7:49 PM, "Lars Francke" <lars.fran...@gmail.com<mailto: > >> >> lars.fran...@gmail.com>> wrote: > >> >> > >> >> >Does anyone have anything further to add here? Looking forward to a > >>few > >> >> >more opinions. > >> >> > > >> >> >On Fri, Dec 11, 2015 at 3:52 PM, Lars Francke > >><lars.fran...@gmail.com > >> >> <mailto:lars.fran...@gmail.com>> > >> >> >wrote: > >> >> > > >> >> >> Hi, > >> >> >> > >> >> >> thanks Larry. > >> >> >> > >> >> >> Lars provided the Knox community with some feedback into our > >> >>development > >> >> >>> practices and JIRA usage [1]. > >> >> >>> > >> >> >>> I wanted to bring up a DISCUSS thread on how our CTR policy may > >>or > >> >>may > >> >> >>> not relate to a couple points made in his feedback. In > >>particular: > >> >> >>> > >> >> >>> 1. Whether a CTR based policy should require actual patches > >> >>attached to > >> >> >>> every JIRA or does the git link to a commit provide the same > >> >>ability to > >> >> >>> review post commit > >> >> >>> > >> >> >> > >> >> >> I think having a patch attached before commit is a good thing > >> >>because: > >> >> >> * It allows feedback before a change goes in. In my experience > >>that > >> >>is > >> >> >> easier to change than committed code > >> >> >> * It allows looking at the exact change set in a standardised form > >> >>(diff > >> >> >> format) without having to use whatever web frontend (pure Git, > >> >>Github, > >> >> ...) > >> >> >> is currently being used (so a patch file is useful even when only > >> >> attached > >> >> >> after committing)[1] > >> >> >> * Should the Git web interface change (or be down) at some point > >>in > >> >>the > >> >> >> future all those links might go stale (say Apache switches to > >>Github > >> >>or > >> >> >> Gitlab or whatever) > >> >> >> > >> >> >> The downsides I can come up with is the extra work required to > >>attach > >> >> the > >> >> >> patch (before or after commit). > >> >> >> > >> >> >> > >> >> >>> 2. Whether a cool off period of 24 hrs would encourage more > >>external > >> >> >>> contributions and if so, how > >> >> >>> > >> >> >> > >> >> >> This one is probably a matter of weighing off between fast > >>iteration > >> >>and > >> >> >> possible feedback on patches. I assume with a project the size of > >> >>Knox > >> >> >> there is not much feedback coming in for each patch so the > >>benefits > >> >>of > >> >> >> immediate commits probably outweigh the (assumed) benefits of > >> >>waiting. > >> >> >> > >> >> >> My personal opinion though is that a wait period is a good thing. > >> >>I've > >> >> >> been bitten (and frustrated) in the past by reviews that have been > >> >> ignored > >> >> >> by certain communities/members/companies where it was clear that a > >> >> release > >> >> >> schedule had to be met. Ignoring reviews is easier with code > >>that's > >> >> already > >> >> >> been committed. Again this is my personal (bad) experience and I > >> >>have no > >> >> >> reason to believe that the Knox community behaves the same. A cool > >> >>off > >> >> >> period doesn't mean that reviews are mandatory, it just > >> >>invites/allows > >> >> >> feedback in my opinion. > >> >> >> > >> >> >> One "real" benefit for pre-commit reviews is that there's tools > >> >> available > >> >> >> and in use at Apache for that (Reviewboard and JIRA to a degree) > >>but > >> >> >> there's currently no tooling support for post-commit reviews. > >> >> >> > >> >> >> Caveat to all of this: I'm only here for a few days a year > >>probably > >> >>and > >> >> >> won't contribute much if at all so you should decide carefully > >> >>whether > >> >> you > >> >> >> want to change working practices for an unknown benefit. > >> >> >> > >> >> >> Cheers, > >> >> >> Lars > >> >> >> > >> >> >> [1] I know that the current web interface has an option to > >>download > >> >>the > >> >> >> patch but it's a different process than most other "Hadoop > >>related" > >> >> >> projects. > >> >> >> > >> >> > >> >> > >> > >> > >
