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