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

[Bruce Richardson <bruce.richardson@intel.com>]

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