Re: [DISCUSS] Addressing Lingering Pull Requests

[Mike Thomsen <mi...@gmail.com>]

It looks like there are at least 10 new processors and services in the
backlog, and quite a few modifications to existing ones.

Something I think would really help here is to expand the scope of
requirements for submitting a new processor for code review to include:

1. docker-compose file that sets up a complete development environment w/
appropriate port mappings that just work when used with NiFi running
outside of Docker on the reviewer's machine.

2. A sample flow, a really KISS example w/ GenerateFlowFile or something
that spits out a query or sample that can let the reviewer really see the
submitter's idea of what the input should look like.

Whether those are stored on a Wiki or in the Git repo doesn't really
matter. I think submitting those artifacts will really reduce the burden of
quickly going from "LGTM, JUnits seem to run" to "OK, I see it actually
running as expected."

On Tue, Jan 30, 2018 at 12:38 AM, James Wing <jv...@gmail.com> wrote:

> This is a great idea, Mark, thanks for proposing it.  30 days after last
> review comment seems like a good, enforceable standard.
>
> James
>
> On Mon, Jan 29, 2018 at 8:29 AM, Mark Payne <ma...@hotmail.com> wrote:
>
> > All,
> >
> > We do from time to time go through the backlog of PR's that need to be
> > reviewed and
> > start a "cleansing" process, closing out any old PR's that appear to have
> > stalled out.
> > When we do this, though, we typically will start sending out e-mails
> > asking if there are
> > any stalled PR's that we shouldn't close and start trying to decipher
> > which ones are okay
> > to close out and which ones are not. This puts quite an onus on the
> > committer who is
> > trying to clean this up. It also can result in having a large number of
> > outstanding Pull Requests,
> > which I believe makes the community look bad because it gives the
> > appearance that we are
> > not doing a good job of being responsive to Pull Requests that are
> > submitted.
> >
> > I would like to propose that we set a new "standard" that is: if we have
> > any Pull Request
> > that has been stalled (and by "stalled" I mean a committer has reviewed
> > the PR and did
> > not merge but asked for clarifications or modifications and the
> > contributor has not pushed
> > any new commit or responded to the comments) for at least 30 days, that
> we
> > go ahead
> > and close the Pull Request (after commenting on the PR that it is being
> > closed due to a lack
> > of activity and that the contributor is more than welcome to open a new
> PR
> > if necessary).
> >
> > I feel like this gives contributors enough time to address concerns and
> it
> > is simple enough
> > to create a new Pull Request if the need arises. Alternatively, if the
> > contributor realizes that
> > they need more time, they can simply comment on the PR that they are
> still
> > interested in
> > working on it but just need more time, and the simple act of commenting
> > will mean that the
> > PR is no longer stalled, as defined above.
> >
> > Any thoughts on such a proposal? Any better alternatives that people have
> > in mind?
> >
> > Thanks
> > -Mark
>

Re: [DISCUSS] Addressing Lingering Pull Requests

[Matt Burgess <ma...@apache.org>]

I agree with Andy on the Docker point, I think it could be too high a
barrier for contribution in some cases. However I think we can build
out / extend a "new component" section of the PR template that has
more best practices, recommendations, suggestions.

I like the idea of the reviewer providing a sample template, perhaps
via Gist or something, but that should probably start as a
recommendation and not a requirement. If a reviewer is unfamiliar with
the new component (or its third-party libraries), they can always ask
the author if they have a test flow they can share, and hopefully the
practice will become status quo organically.

Another thing we could add is checkbox(es) around standard efforts
associated with the addition of new components/bundles. Specifically
if you add new NARs, you have to add them to the nifi-assembly POM and
their versions to the top-level POM, not to mention the bundle to the
nifi-nar-bundles POM.

Regards,
Matt


On Thu, Feb 1, 2018 at 10:20 AM, Andy LoPresto <al...@apache.org> wrote:
> Mike,
>
> I think that would be awesome for reviewers (and that is where most of my
> time is spent on the review side), but I also think that sets a really high
> bar for contribution. Many of the users who submit pull requests are
> first-time contributors or new to the project, and I believe the easier we
> can make contributing, the more we will grow and strengthen our community.
> Someone might have experience with a weird protocol or multithreading and be
> able to offer expertise there, but not have the familiarity with Docker. For
> experienced users contributing advanced functionality though, I think it
> would be excellent and definitely streamline the review process.
>
>
> Andy LoPresto
> alopresto@apache.org
> alopresto.apache@gmail.com
> PGP Fingerprint: 70EC B3E5 98A6 5A3F D3C4  BACE 3C6E F65B 2F7D EF69
>
> On Feb 1, 2018, at 9:31 AM, Mike Thomsen <mi...@gmail.com> wrote:
>
> It looks like there are at least 10 new processors and services in the
> backlog, and quite a few modifications to existing ones.
>
> Something I think would really help here is to expand the scope of
> requirements for submitting a new processor for code review to include:
>
> 1. docker-compose file that sets up a complete development environment w/
> appropriate port mappings that just work when used with NiFi running
> outside of Docker on the reviewer's machine.
>
> 2. A sample flow, a really KISS example w/ GenerateFlowFile or something
> that spits out a query or sample that can let the reviewer really see the
> submitter's idea of what the input should look like.
>
> Whether those are stored on a Wiki or in the Git repo doesn't really
> matter. I think submitting those artifacts will really reduce the burden of
> quickly going from "LGTM, JUnits seem to run" to "OK, I see it actually
> running as expected."
>
> On Tue, Jan 30, 2018 at 12:38 AM, James Wing <jv...@gmail.com> wrote:
>
> This is a great idea, Mark, thanks for proposing it.  30 days after last
> review comment seems like a good, enforceable standard.
>
> James
>
> On Mon, Jan 29, 2018 at 8:29 AM, Mark Payne <ma...@hotmail.com> wrote:
>
> All,
>
> We do from time to time go through the backlog of PR's that need to be
> reviewed and
> start a "cleansing" process, closing out any old PR's that appear to have
> stalled out.
> When we do this, though, we typically will start sending out e-mails
> asking if there are
> any stalled PR's that we shouldn't close and start trying to decipher
> which ones are okay
> to close out and which ones are not. This puts quite an onus on the
> committer who is
> trying to clean this up. It also can result in having a large number of
> outstanding Pull Requests,
> which I believe makes the community look bad because it gives the
> appearance that we are
> not doing a good job of being responsive to Pull Requests that are
> submitted.
>
> I would like to propose that we set a new "standard" that is: if we have
> any Pull Request
> that has been stalled (and by "stalled" I mean a committer has reviewed
> the PR and did
> not merge but asked for clarifications or modifications and the
> contributor has not pushed
> any new commit or responded to the comments) for at least 30 days, that
>
> we
>
> go ahead
> and close the Pull Request (after commenting on the PR that it is being
> closed due to a lack
> of activity and that the contributor is more than welcome to open a new
>
> PR
>
> if necessary).
>
> I feel like this gives contributors enough time to address concerns and
>
> it
>
> is simple enough
> to create a new Pull Request if the need arises. Alternatively, if the
> contributor realizes that
> they need more time, they can simply comment on the PR that they are
>
> still
>
> interested in
> working on it but just need more time, and the simple act of commenting
> will mean that the
> PR is no longer stalled, as defined above.
>
> Any thoughts on such a proposal? Any better alternatives that people have
> in mind?
>
> Thanks
> -Mark
>
>
>

Re: [DISCUSS] Addressing Lingering Pull Requests

[Andy LoPresto <al...@apache.org>]

Mike,

I think that would be awesome for reviewers (and that is where most of my time is spent on the review side), but I also think that sets a really high bar for contribution. Many of the users who submit pull requests are first-time contributors or new to the project, and I believe the easier we can make contributing, the more we will grow and strengthen our community. Someone might have experience with a weird protocol or multithreading and be able to offer expertise there, but not have the familiarity with Docker. For experienced users contributing advanced functionality though, I think it would be excellent and definitely streamline the review process.


Andy LoPresto
alopresto@apache.org
alopresto.apache@gmail.com
PGP Fingerprint: 70EC B3E5 98A6 5A3F D3C4  BACE 3C6E F65B 2F7D EF69

> On Feb 1, 2018, at 9:31 AM, Mike Thomsen <mi...@gmail.com> wrote:
> 
> It looks like there are at least 10 new processors and services in the
> backlog, and quite a few modifications to existing ones.
> 
> Something I think would really help here is to expand the scope of
> requirements for submitting a new processor for code review to include:
> 
> 1. docker-compose file that sets up a complete development environment w/
> appropriate port mappings that just work when used with NiFi running
> outside of Docker on the reviewer's machine.
> 
> 2. A sample flow, a really KISS example w/ GenerateFlowFile or something
> that spits out a query or sample that can let the reviewer really see the
> submitter's idea of what the input should look like.
> 
> Whether those are stored on a Wiki or in the Git repo doesn't really
> matter. I think submitting those artifacts will really reduce the burden of
> quickly going from "LGTM, JUnits seem to run" to "OK, I see it actually
> running as expected."
> 
> On Tue, Jan 30, 2018 at 12:38 AM, James Wing <jv...@gmail.com> wrote:
> 
>> This is a great idea, Mark, thanks for proposing it.  30 days after last
>> review comment seems like a good, enforceable standard.
>> 
>> James
>> 
>> On Mon, Jan 29, 2018 at 8:29 AM, Mark Payne <ma...@hotmail.com> wrote:
>> 
>>> All,
>>> 
>>> We do from time to time go through the backlog of PR's that need to be
>>> reviewed and
>>> start a "cleansing" process, closing out any old PR's that appear to have
>>> stalled out.
>>> When we do this, though, we typically will start sending out e-mails
>>> asking if there are
>>> any stalled PR's that we shouldn't close and start trying to decipher
>>> which ones are okay
>>> to close out and which ones are not. This puts quite an onus on the
>>> committer who is
>>> trying to clean this up. It also can result in having a large number of
>>> outstanding Pull Requests,
>>> which I believe makes the community look bad because it gives the
>>> appearance that we are
>>> not doing a good job of being responsive to Pull Requests that are
>>> submitted.
>>> 
>>> I would like to propose that we set a new "standard" that is: if we have
>>> any Pull Request
>>> that has been stalled (and by "stalled" I mean a committer has reviewed
>>> the PR and did
>>> not merge but asked for clarifications or modifications and the
>>> contributor has not pushed
>>> any new commit or responded to the comments) for at least 30 days, that
>> we
>>> go ahead
>>> and close the Pull Request (after commenting on the PR that it is being
>>> closed due to a lack
>>> of activity and that the contributor is more than welcome to open a new
>> PR
>>> if necessary).
>>> 
>>> I feel like this gives contributors enough time to address concerns and
>> it
>>> is simple enough
>>> to create a new Pull Request if the need arises. Alternatively, if the
>>> contributor realizes that
>>> they need more time, they can simply comment on the PR that they are
>> still
>>> interested in
>>> working on it but just need more time, and the simple act of commenting
>>> will mean that the
>>> PR is no longer stalled, as defined above.
>>> 
>>> Any thoughts on such a proposal? Any better alternatives that people have
>>> in mind?
>>> 
>>> Thanks
>>> -Mark
>>