From mboxrd@z Thu Jan 1 00:00:00 1970 From: Bruce Richardson Subject: Re: [dpdk-techboard] decision process and DPDK scope Date: Thu, 9 Feb 2017 12:20:47 +0000 Message-ID: <20170209122047.GA327480@bricha3-MOBL3.ger.corp.intel.com> References: <1667864.GflPPoyiWF@xps13> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: dev@dpdk.org, techboard@dpdk.org To: Thomas Monjalon Return-path: Content-Disposition: inline In-Reply-To: <1667864.GflPPoyiWF@xps13> List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" On Thu, Feb 09, 2017 at 12:11:39PM +0100, Thomas Monjalon wrote: > Hi all, > Hi Thomas, thanks for kicking off the discussion here. > When DPDK was a small project, it was easy to propose a major change, > get feedback from the few contributors and figure a decision. > It had the drawback of the lack of various point of views. > So we probably made some quick and wrong decisions. > > As the community is growing, we need to improve the decision process > to make sure the responsibilities are well shared and represent the > diversity of the community. > > There has been a recent failure in this process. I would like to show > it as an example of things to better solve. > During last August, a patch was sent: "add bit-rate metrics to xstats". > After more thoughts, a v2 was sent in October: "expanded statistic reporting". > Starting from this version, the idea was to add completely new libraries. > Nobody (including me) asked why we should maintain these things in DPDK. > I have just realized that there was neither discussion on the need for these > libraries nor on the DPDK scope. I feel the DPDK scope (and API in general) > should be better owned by the community. So I took the decision to not > integrate these patches in 17.02 and I'm sorry about that. > It is a failure to not give good feedbacks on time. > It is a failure to not ask the right questions. > It is a failure to not have more attention on a new feature. > It is a failure to take such decision alone. > I can't disagree here, like many problems there tends to be more than one failure behind it. However, it does bring up a number of issues, some of which you touch on below, but others on which are not yet raised. > I think we can use this case to avoid seeing it again in the future. > I suggest that the technical board should check whether every new proposed > features are explained, discussed and approved enough in the community. > If needed, the technical board meeting minutes will give some lights to > the threads which require more attention. > Before adding a new library or adding a major API, there should be > some strong reviews which include discussing the DPDK scope. > The bigger question here is the default position of the DPDK community - default accept, or default reject. Your statements above all are very much keeping in the style of default reject i.e. every patch or change suggested is assumed to be unfit for acceptance unless reviewed in detail to prove beyond doubt otherwise. I believe that we should change this default position, as I think that reject by default is hurting the community and will continue to do so. NOTE: I am not suggesting that we allow all code in with zero review, but I am suggesting that if something has been reviewed and acked by at least one reviewer it should be automatically accepted unless some other reviewed objects in a TIMELY manner. I think it's helpful to take a look at the implications of the two approaches. With our current policy: * a new feature requires an undefined number of reviews to make it into the release. The amount of review depends on a number of factors. * the responsibility to get those reviews depends entirely on the submitter to chase up reviews to ensure his patch gets merged. For anyone new to the community, this is a difficult task to know a) how many reviews are needed b) who those reviewers need to be to have enough credibility to get the patch accepted * at any time, up until the day the patch is merged, any member of the community can come back with feedback, so one can never be sure that it gets put in, until it is actually in. This makes planning work for inclusion in a DPDK release very challenging. * there is little motivation for other community reviewers to review patches in a timely manner as they have, in many cases, up until the merge deadline to raise any objections to a patch. However, if we switch our policy to a default accept with a minimum of one review, plus e.g. a 2 week additional review period: * there is a known number of reviews minimum that needs to be got, and everyone is aware of it. * the responsibility is now divided between the submitter and the community. The submitter is responsible for getting someone in the community to review their patch. However, thereafter the responsibility is on the rest of the community to object in a timely manner. * anyone who is concerned about a new feature is now motivated to review quickly or else their feedback will be rejected as coming too late. This will help avoid a sudden rush of feedback at the end. * once the additional review period has passed, a submitter knows they can move on to work on other things as their patch will be merged, and anyone else wanting to build on that work can do so safe in the knowledge that their dependencies will be met. I think the latter is a much better model, as it shares responsibilities and should encourage earlier reviews. If a review with a serious objection does come late then the reviewer has two options: * submit a patch to roll back the offending feature * submit a patch to fix the offending feature, or work with the original submitter to fix it. In both cases, there is more responsibility on the objector to actually follow up on their objections rather than just complaining over email. Again, I view this as a good thing. Overall, the suggestions I make above are just guidelines, I'm happy enough if we decide that for new libs we need 2 acks from existing committers before it gets accepted, or that we have an additional proviso that a patch cannot break the build (provided that we do allow a small fix-window e.g. 2 days to allow minor fixes like that to be done by submitter). However, what I feel very strongly about, is that we MUST provide a clear set of easily-achieved criteria, which, when met, mean that a maintainer MUST merge a feature. Apologies for the long email, but I think we need to have this discussion. As for the specific case, I feel that the new lib should be merged, irrespective of quality or scope or other concerns, but simply because nobody objected in time, and it had been reviewed and acked. If it turns out to be a mistake to merge it, that's the fault of those (including me) who didn't review it in time. The feature submitter should not be punished by having the feature rejected for my mistake in not taking time to review it! > Openness of a large community is proven by its active feedbacks. +1. Let's incentivize early feedback. Regards, /Bruce