[dpdk-dev] decision process and DPDK scope

[dpdk-dev] decision process and DPDK scope

[Thomas Monjalon]

Hi all,

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

Openness of a large community is proven by its active feedbacks.

Re: [dpdk-dev] decision process and DPDK scope

[O'Driscoll, Tim]

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> 
> Hi all,
> 
> 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 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.

I assume you don't mean every new feature, just those that involve major changes (new libraries, new/modified APIs etc.). Is that correct?

> 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.
> 
> Openness of a large community is proven by its active feedbacks.

+1

At the moment, when there's no feedback on an RFC or patch set, there's no way of knowing whether that means people are happy with it or that nobody has reviewed it. Using the Tech Board to highlight RFCs/patch sets that require more review is a good idea.

Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope

[Bruce Richardson]

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

Re: [dpdk-dev] decision process and DPDK scope

[Thomas Monjalon]

2017-02-09 11:54, O'Driscoll, Tim:
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Thomas Monjalon
> > I suggest that the technical board should check whether every new
> > proposed features are explained, discussed and approved enough in
> > the community.
> 
> I assume you don't mean every new feature, just those that involve
> major changes (new libraries, new/modified APIs etc.). Is that correct?

Yes, it is not about drivers. It is more about API.

> > 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.
> > 
> > Openness of a large community is proven by its active feedbacks.
> 
> +1
> 
> At the moment, when there's no feedback on an RFC or patch set, there's no way of knowing whether that means people are happy with it or that nobody has reviewed it. Using the Tech Board to highlight RFCs/patch sets that require more review is a good idea.

Yes it is my thought: we should have several explicit agreements for
important patches.

Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope

[Stephen Hemminger]

On Thu, 9 Feb 2017 12:20:47 +0000
Bruce Richardson <bruce.richardson@intel.com> wrote:

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

I agree but in a more assertive manner. The maintainer should be the default
and active reviewer of all submissions. Like other projects the maintainers job
is to review and accept (or provide constructive feedback). Otherwise the
job could just by done by some manager.

But recently, I have changed my mind. The current DPDK project model is not
scaling well. After hearing some of the arguments in favor of a multiple
committer model (see "Maintainers Don't Scale" )
https://kernel-recipes.org/en/2016/talks/maintainers-dont-scale/

And comments on lwn:
https://lwn.net/Articles/703005/

Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope

[Bruce Richardson]

On Thu, Feb 09, 2017 at 02:49:05PM -0800, Stephen Hemminger wrote:
> On Thu, 9 Feb 2017 12:20:47 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > > 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 autom
> 
> I agree but in a more assertive manner. The maintainer should be the default
> and active reviewer of all submissions. Like other projects the maintainers job
> is to review and accept (or provide constructive feedback). Otherwise the
> job could just by done by some manager.
> 
> But recently, I have changed my mind. The current DPDK project model is not
> scaling well. After hearing some of the arguments in favor of a multiple
> committer model (see "Maintainers Don't Scale" )
> https://kernel-recipes.org/en/2016/talks/maintainers-dont-scale/
> 
> And comments on lwn:
> https://lwn.net/Articles/703005/
> 
Might it be worthwhile to try out having 2 or 3 committers to each tree
and see how it works? From the presentation you link too, the claim is
that moving from 1 to 2 is the hardest, and expanding beyond that
becomes easier.

/Bruce

Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope

[Thomas Monjalon]

2017-02-10 15:54, Bruce Richardson:
> On Thu, Feb 09, 2017 at 02:49:05PM -0800, Stephen Hemminger wrote:
> > On Thu, 9 Feb 2017 12:20:47 +0000
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > 
> > > > 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.

It is hurting because there is no clear explanation of the process.

> > > 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 see an issue with "automatic" decision after a period of time.
It puts a lot of pressure on the community to check everything.
I agree we should state this kind of default. But we should add two
exceptions:
	- new API or API change
	- a maintainer explicitly ask for a techboard discussion


> > I agree but in a more assertive manner. The maintainer should be the default
> > and active reviewer of all submissions. Like other projects the maintainers job
> > is to review and accept (or provide constructive feedback). Otherwise the
> > job could just by done by some manager.
> > 
> > But recently, I have changed my mind. The current DPDK project model is not
> > scaling well. After hearing some of the arguments in favor of a multiple
> > committer model (see "Maintainers Don't Scale" )
> > https://kernel-recipes.org/en/2016/talks/maintainers-dont-scale/
> > 
> > And comments on lwn:
> > https://lwn.net/Articles/703005/
> > 
> Might it be worthwhile to try out having 2 or 3 committers to each tree
> and see how it works? From the presentation you link too, the claim is
> that moving from 1 to 2 is the hardest, and expanding beyond that
> becomes easier.

I think the first thing to improve is the decision process.
Increasing the number of committers, without agreeing on a clear
decision process, would make things worse.

Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope

[Bruce Richardson]

On Fri, Feb 10, 2017 at 06:23:11PM +0100, Thomas Monjalon wrote:
> 2017-02-10 15:54, Bruce Richardson:
> > On Thu, Feb 09, 2017 at 02:49:05PM -0800, Stephen Hemminger wrote:
> > > On Thu, 9 Feb 2017 12:20:47 +0000
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > 
> > > > > 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.
> 
> It is hurting because there is no clear explanation of the process.
> 
> > > > 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 see an issue with "automatic" decision after a period of time.
> It puts a lot of pressure on the community to check everything.
> I agree we should state this kind of default. But we should add two
> exceptions:
> 	- new API or API change
> 	- a maintainer explicitly ask for a techboard discussion
>
Ok, I will accept the referral to the tech board as a reason to not
automatically apply, but I disagree on the API one. For all types of
changes we need to clearly document the number of reviews needed to get
the patches in, and who needs to agree. Once those conditions are met,
we must have a mandatory merge. The difference between adding a new API
and adding an internal code change should only be in the conditions
required for mandatory merge. There must still be an automatic decision
after a set period of time, otherwise we will still see issues with
people leaving reviews till the last minute and then flagging problems
with a patch blocking its merge.

/Bruce

Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
> Sent: Thursday, February 9, 2017 10:49 PM
> To: Richardson, Bruce <bruce.richardson@intel.com>
> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; dev@dpdk.org;
> techboard@dpdk.org
> Subject: Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope
> 
> On Thu, 9 Feb 2017 12:20:47 +0000
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
> > > 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 autom
> 
> I agree but in a more assertive manner. The maintainer should be the
> default and active reviewer of all submissions. Like other projects the
> maintainers job is to review and accept (or provide constructive
> feedback). Otherwise the job could just by done by some manager.
> 
> But recently, I have changed my mind. The current DPDK project model is
> not scaling well. After hearing some of the arguments in favor of a
> multiple committer model (see "Maintainers Don't Scale" ) https://kernel-
> recipes.org/en/2016/talks/maintainers-dont-scale/
> 
> And comments on lwn:
> https://lwn.net/Articles/703005/

Hi Stephen,

That is an interesting case study. Perhaps it is something we could trial in 17.05 on one or more of the sub-trees.

John

Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope

[Wiles, Keith]

> On Feb 13, 2017, at 9:21 AM, Mcnamara, John <john.mcnamara@intel.com> wrote:
> 
> 
> 
>> -----Original Message-----
>> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Stephen Hemminger
>> Sent: Thursday, February 9, 2017 10:49 PM
>> To: Richardson, Bruce <bruce.richardson@intel.com>
>> Cc: Thomas Monjalon <thomas.monjalon@6wind.com>; dev@dpdk.org;
>> techboard@dpdk.org
>> Subject: Re: [dpdk-dev] [dpdk-techboard] decision process and DPDK scope
>> 
>> On Thu, 9 Feb 2017 12:20:47 +0000
>> Bruce Richardson <bruce.richardson@intel.com> wrote:
>> 
>>>> 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 autom
>> 
>> I agree but in a more assertive manner. The maintainer should be the
>> default and active reviewer of all submissions. Like other projects the
>> maintainers job is to review and accept (or provide constructive
>> feedback). Otherwise the job could just by done by some manager.
>> 
>> But recently, I have changed my mind. The current DPDK project model is
>> not scaling well. After hearing some of the arguments in favor of a
>> multiple committer model (see "Maintainers Don't Scale" ) https://kernel-
>> recipes.org/en/2016/talks/maintainers-dont-scale/
>> 
>> And comments on lwn:
>> https://lwn.net/Articles/703005/
> 
> Hi Stephen,
> 
> That is an interesting case study. Perhaps it is something we could trial in 17.05 on one or more of the sub-trees.

+1 What sub-trees would be the best place to start and then who would be best suited to the role?

I was thinking net-next would be a good place to start.

> 
> John
> 
> 

Regards,
Keith