nifi-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andy LoPresto <alopre...@apache.org>
Subject Re: [DISCUSS] Stale PRs
Date Tue, 18 Sep 2018 16:51:54 GMT
Pierre,

I’m a +0 [1].

[1] https://www.apache.org/foundation/voting.html#expressing-votes-1-0-1-and-fractions

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

> On Sep 18, 2018, at 7:57 AM, Otto Fowler <ottobackwards@gmail.com> wrote:
> 
> I didn’t mean to say that the Metron way is better, I’m sorry if it came
> off as that.
> My main point was rather that we side-stepped a lot of things by limiting
> the case to ‘contributor non-responsiveness’.
> 
> Had we known about the github integration possibilities  at the time, we
> certainly would have considered that implementation
> as opposed to the more manual process we have now.
> 
> 
> 
> On September 18, 2018 at 09:39:56, Pierre Villard (
> pierre.villard.fr@gmail.com) wrote:
> 
> Otto, I think using the embedded option offered by Github is better as I
> don't think submitting requests to Apache Infra for this is great.
> 
> What would be the best way forward on this subject?
> Andy, do you still have concerns and/or comments based on the feedback?
> Should it go through a formal VOTE thread?
> 
> Happy to take care of it if there is a consensus.
> 
> Thanks,
> Pierre
> 
> 
> Le dim. 16 sept. 2018 à 15:38, Otto Fowler <ottobackwards@gmail.com> a
> écrit :
> 
>> In Metron we just put something like this in place, but not using a bot.
>> We limit it to PR’s where the contributor has gone inactive.
>> https://cwiki.apache.org/confluence/display/METRON/Development+Guidelines
>> 
>> 2.6.1 Inactive Pull Requests
>> 
>> Contributions can often take a significant amount of time to complete the
>> code review process. This process requires active participation from the
>> contributor. If the contributor is unable to actively participate, the
> pull
>> request is unlikely to successfully complete this process.
>> 
>> Pull Requests that have failed to receive active participation from the
>> contributor for an extended period of time risk being abandoned. Any
>> committer can submit a request for Apache Infra to close a pull request
>> that has been abandoned according to the following guidelines.
>> 
>> - A pull request is 'inactive' if no comments or updates have been made
>> by the contributor in the previous 4 weeks.
>> 
>> 
>> - For any 'inactive' pull request, a committer can request from the
>> contributor justification for keeping the pull request open.
>> 
>> 
>> - The committer's request should be made as a public comment on the pull
>> request. The committer should refer the contributor to these development
>> guidelines for inactive pull requests.
>> 
>> 
>> - If the contributor publically responds to the request, the pull
>> request is no longer consider 'inactive'.
>> 
>> 
>> - If the contributor does not respond to the request within 2 weeks, the
>> pull request is considered 'abandoned'.
>> 
>> 
>> - A committer can cast a -1 vote on any 'abandoned' pull request using
>> these development guidelines as justification.
>> 
>> 
>> - A committer can submit a request to Apache Infra to close the
>> 'abandoned' pull request based on this -1 vote.
>> 
>> 
>> 
>> On September 15, 2018 at 22:22:17, Mike Thomsen (mikerthomsen@gmail.com)
>> wrote:
>> 
>> I am in favor of these changes. One thing we should consider is looking
> for
>> old PRs that are close enough to being done that we could rebase them,
>> clean them up a little and add them to master. Just a thought.
>> 
>> Thanks,
>> 
>> Mike
>> 
>> On Sat, Sep 15, 2018 at 1:34 PM Mark Payne <markap14@hotmail.com> wrote:
>> 
>>> I'm 100% on-board here. I brought up this same topic a couple of months
>>> ago,
>>> but the thread kind of digressed (as these things tend to do on large
>>> mailing lists).
>>> I am in favor of a 30 day period with a reminder that gives the
>>> contributor an extra
>>> week before closing the PR. If the contributor is simply busy and not
>> able
>>> to finish up
>>> for a while, a simple comment to that effect would allow the PR to stay
>>> open. At least
>>> this way we know whether a PR is in-progress or just lingering and will
>>> never get
>>> any progress.
>>> 
>>> While there are times that the committers are at fault, I think that's
> a
>>> separate discussion
>>> that we can have. Both sides of the equation have to be addressed, and
>>> that should not
>>> prevent us from closing out stale PR's.
>>> 
>>> That being said, I think closing out stale PR's will actually improve
> the
>>> committers' review
>>> rate. I sometimes start looking through the list of PR's to review and
>>> then get overwhelmed
>>> because there are so many of them right now, and almost all of them
> have
>> a
>>> comment of
>>> some sort on them. Often I have no idea if the PR is still being worked
>> on
>>> or not. If we reduce
>>> the number of open PR's down to only those that are still being worked,
>>> it's a lot less overwhelming
>>> for the committers to look through and see what needs to be reviewed.
>>> 
>>> It's also worth nothing that this is not just something that Beam does.
> I
>>> know Kubernetes has a similar
>>> mechanism in place, and I'm guessing that this is a pretty common
>> practice
>>> in general. And one that
>>> I think will definitely help out both committers and contributors and
>> make
>>> the project more approachable
>>> from those who are interested in getting involved.
>>> 
>>> Thanks
>>> -Mark
>>> 
>>> 
>>>> On Sep 15, 2018, at 12:08 PM, Jeremy Dyer <jdye64@gmail.com> wrote:
>>>> 
>>>> Andy - That’s a good point. What I had in my mind was sort of like
> this
>>> ....
>>>> 
>>>> - After 30 days alert is sent to author if no activity/comment not
> just
>>> commits.They would still have something like a week
>>>> - If code is complicated they don’t have to finish it just simply
>>> comment on PR to stop it from being closed
>>>> - I like this because when they comment they can say things like.
>> “Sorry
>>> this is taking longer because of problem XYZ I’m having” others in the
>>> community can see this and offer input so it helps on collaboration
>>>> - This also helps people watching the PRs and interested in using
> them
>>> have a much more clear and accurate understanding of where the PR
>> actually
>>> stands progress wise so they can more accurately determine when it will
>> be
>>> available to them
>>>> - To your last point, which is a good one, they would simply comment
>>> with a gentle reminder that they would like for someone to review.
>>>> 
>>>> Thoughts? I’m sure I’m missing something there but that’s sort of how
> I
>>> imagine it working
>>>> 
>>>> - Jeremy Dyer
>>>> 
>>>> Thanks - Jeremy Dyer
>>>> 
>>>> ________________________________
>>>> From: Andy LoPresto <alopresto.apache@gmail.com>
>>>> Sent: Saturday, September 15, 2018 11:57 AM
>>>> To: dev@nifi.apache.org
>>>> Subject: Re: [DISCUSS] Stale PRs
>>>> 
>>>> Jeremy,
>>>> 
>>>> What about in the scenario where the submitter does everything and we
>>> (the committers) are slow? I’m not saying we shouldn’t try to fix all
> the
>>> problems, just that I see a lot more of the latter happening.
>>>> 
>>>> Andy LoPresto
>>>> alopresto@apache.org
>>>> alopresto.apache@gmail.com
>>>> PGP Fingerprint: 70EC B3E5 98A6 5A3F D3C4 BACE 3C6E F65B 2F7D EF69
>>>> 
>>>>> On Sep 15, 2018, at 08:51, Pierre Villard <
>> pierre.villard.fr@gmail.com>
>> 
>>> wrote:
>>>>> 
>>>>> Andy,
>>>>> 
>>>>> Totally get your points. I imagine that introducing this approach
>> would
>>>>> help keeping dynamic exchanges on pull requests.
>>>>> 
>>>>> In case a PR needs complex/time consuming work (or in case the
> author
>> is
>>>>> just not in a position to process comments), I think we could have
> two
>>>>> approaches:
>>>>> - the PR is considered stale after 60 days but is actually closed
> one
>>> week
>>>>> later. I think it leaves time for someone (ideally the author) to
>>> comment
>>>>> and give an update so that the PR is not considered stale anymore,
> no?
>>>>> - for important PRs, it's possible to "remove" this mechanism using
>>>>> specific labels but I guess we would have to ask ASF infra if we
> want
>> to
>>>>> have rights to add labels on PRs (?)
>>>>> 
>>>>> Pierre
>>>>> 
>>>>> 
>>>>> 
>>>>> 
>>>>> Le sam. 15 sept. 2018 à 17:44, Andy LoPresto <
>>> alopresto.apache@gmail.com> a
>>>>> écrit :
>>>>> 
>>>>>> Pierre,
>>>>>> 
>>>>>> I’m going to delay my response on that proposal while I ask for
> (aka
>>>>>> should gather on my own) some information. Is that really our
>> problem?
>>> By
>>>>>> that, I mean are stale PRs where we are getting bogged down? I am
>> sure
>>>>>> there are some old ones that should be closed out. My larger
> concern
>> is
>>>>>> that even new PRs don’t get reviewed immediately for a number of
>>> reasons.
>>>>>> 
>>>>>> 1. Balance of committers to submissions. As the project continues
> to
>>> grow,
>>>>>> we have far more people providing code than can review it.
>>>>>> 2. Quality of PR. Not that the code is necessarily bad, but the PR
>>> doesn’t
>>>>>> clearly explain the problem and how they are solving it, provide
> test
>>>>>> cases, provide templates or a Docker container if interacting with
> an
>>>>>> external service, etc. All of these things add up to make the cost
> of
>>>>>> reviewing higher.
>>>>>> 3. What PRs cover. Previously, there was still a lot of low-hanging
>>> fruit,
>>>>>> and less complexity. While the project is still fairly cleanly
>>> organized,
>>>>>> many PRs now are less “add this simple functionality” and more
>> “improve
>>>>>> this complicated feature.”
>>>>>> 
>>>>>> I guess I would not have a problem with your proposal, but I do
>> wonder
>>> if
>>>>>> there are more effective ways to reduce the backlog by identifying
>>> other
>>>>>> areas of improvement.
>>>>>> 
>>>>>> Andy LoPresto
>>>>>> alopresto@apache.org
>>>>>> alopresto.apache@gmail.com
>>>>>> PGP Fingerprint: 70EC B3E5 98A6 5A3F D3C4 BACE 3C6E F65B 2F7D EF69
>>>>>> 
>>>>>>> On Sep 15, 2018, at 08:33, Pierre Villard <
>>> pierre.villard.fr@gmail.com>
>>>>>> wrote:
>>>>>>> 
>>>>>>> Hi,
>>>>>>> 
>>>>>>> The number of open PRs is still growing and it could make think
>> people
>>>>>> that
>>>>>>> the project is not healthy/active (even though a quick look at
the
>>> last
>>>>>>> commit date or the commits rate is a clear indication that the
>>> project is
>>>>>>> healthy).
>>>>>>> 
>>>>>>> In order to encourage people to review code and keep active
>>> discussions
>>>>>> on
>>>>>>> the PRs, I suggest to find a way to keep this number as small
as
>>>>>> possible.
>>>>>>> To do so, I'd like to ask the wider community if the approach
> taken
>>> by a
>>>>>>> project like Apache Beam would be a good idea:
>>>>>>> 
>>>>>>> "A pull request becomes stale after its author fails to respond
to
>>>>>>> actionable comments for 60 days. Author of a closed pull request
> is
>>>>>> welcome
>>>>>>> to reopen the same pull request again in the future."
>>>>>>> 
>>>>>>> This approach is managed by a file [1] in the .github directory
of
>> the
>>>>>>> repository.
>>>>>>> 
>>>>>>> What do you think about this approach?
>>>>>>> 
>>>>>>> [1] https://github.com/apache/beam/blob/master/.github/stale.yml
>>>>>>> 
>>>>>>> Pierre
>>>>>> 
>>> 
>>> 
>> 


Mime
View raw message