drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Timothy Farkas <tfar...@mapr.com>
Subject Re: [Vote] Cleaning Up Old PRs
Date Thu, 07 Jun 2018 20:08:03 GMT
Thanks for the feedback Dave / Robert.

So here is revised proposal that does not automatically close a PR, but does include some
automation to alleviate some of the leg work:


 1. Auto reply with information about how to help the contributor to be successful and about
the PR process.

 2. Use probot stale to determine if a PR has been inactive for 3 months (or whatever is the
most popular duration polled in the survey). If so label the PR as inactive. The PR will remain
open, it will just have a friendly little label added to it.

 3. Committers must look at inactive PRs and manually send a mail directly to the contributor
and dev list to do follow up.

 4. If the contributor responds within a month, then great we got back to step 2.

 5. If the contributor fails to respond within a month and the committer does not see a path
forward, the PR is closed.

 6. If the contributor fails to respond but the committer sees value in keeping the PR open
the committer can label the PR as pending, and the PR will remain alive indefinitely.

I want to include some level of automation for a couple of reasons:

 1. Periodically checking PRs if 3 months is up is very boring and very time consuming. If
it is not automated it won't be done and the process will fall apart.
 2. It keeps people honest. Without some guide rails people tend to forget to handle things,
and process / culture decays.

Also there seems to be a misconception about closing PRs. Closing a PR in no way deletes or
permanently removes it. A closed PR will remain on Github forever, is linked to the jira ticket
so is discoverable, and can be reopened anytime by the contributor or by any committer indefinitely.

Please let me know your thoughts.

Thanks,
Tim



________________________________
From: Dave Oshinsky <doshinsky@commvault.com>
Sent: Thursday, June 7, 2018 12:35:04 PM
To: dev
Subject: Re: [Vote] Cleaning Up Old PRs

Makes sense, Aman.  The fact that such PR's exist should cause us to regard automatic removal
of PR's as a risky proposition.  There is no way (currently) for a machine to reliably determine
which PR's have these kinds of qualities.

Regarding my experience, it is not just the fact that the PR was not reviewed for over one
year.  It is the fact that there was complete silence.  Some communication would have been
helpful.  By the time Volodymyr took up the issue, it was a huge, pleasant surprise for me.
 Before that point, I thought the whole VARDECIMAL effort was quite dead, and was hesitant
to put more time into it for the periodic merge to master tip.

One more point of feedback.  A situation where one person starts the effort (with an idea
and a PR), and then a "project insider" takes over the process and makes the rest of the changes
to complete the effort, is not an uncommon one.  To expect a "project outsider", with only
limited detailed knowledge of all of the Drill internals and test procedures to take such
a PR from initial idea through its final state just before commit is just not realistic. 
This kind of "team effort" can take time, and should not be interrupted by automatically removing
the PR.

Thanks,
Dave Oshinsky

________________________________________
From: Aman Sinha <amansinha@apache.org>
Sent: Thursday, June 7, 2018 3:14 PM
To: dev
Subject: Re: [Vote] Cleaning Up Old PRs

I haven't looked at Tim's survey yet but just wanted to respond to Dave
about his experience.  I can assure you that Drill committers would welcome
quality contributions from you or anyone else.   In the case of your PR,
the reason for it getting stuck was basically what you already
mentioned: "there
was simply nobody available to review it".   I would add one more element
to it:  which is the design aspects.  Changes or enhancement to a core data
type does involve more design (and code review) than bug fixes or smaller
feature PRs or for contributions to areas like storage plugins which are
stand-alone.   Until Volodymyr specifically started looking into the
design, it was difficult to get a comprehensive view of the changes needed
for decimal.

Aman

On Thu, Jun 7, 2018 at 11:51 AM, Robert Hou <rhou@mapr.com> wrote:

> The Exchange PR was under active development, but there were some issues
> that could not be resolved at the time.  So it was shelved until someone
> could get some time to resolve those issues.
>
>
> Thanks.
>
>
> --Robert
>
> ________________________________
> From: Robert Hou <rhou@mapr.com>
> Sent: Thursday, June 7, 2018 11:46 AM
> To: dev@drill.apache.org
> Subject: Re: [Vote] Cleaning Up Old PRs
>
> On a related note, someone created a PR to resolve some Exchange issues a
> year ago.  It has been dormant since then, and the original author is
> probably not going to push it forward.  However, a second person has picked
> it up now because we need to resolve the issue.  There is a lot of good
> work in that PR, and it has provided a great starting point.
>
>
> I'm not against cleaning up old PRs.  But I am not sure it is easy to
> automate without losing some good work.
>
>
> Thanks.
>
>
> --Robert
>
> ________________________________
> From: Dave Oshinsky <doshinsky@commvault.com>
> Sent: Thursday, June 7, 2018 11:34 AM
> To: dev@drill.apache.org
> Subject: Re: [Vote] Cleaning Up Old PRs
>
> Hi Tim,
> Everyone's time is constrained, so I doubt that it will always be possible
> to give "timely" reviews to PR's, especially complex ones, or ones
> regarding problems that are not regarded as high priority.  I suggest these
> changes to your scheme:
>
> 1) Once a PR reaches the 3 months point, send an email to the list and
> directly to the PR creator that the PR will automatically be closed in 1
> more month if specific actions are not taken.  The PR creator is less
> likely to miss an email that is sent directly to him/her.
> 2) Automatic removals should not be executed until an administrator has
> approved it.  In other words, it should not be completely automatic,
> without a human in the loop.
> 3) PR's that are closed (either automatically or not) should remain in the
> system for some time (with "reopen" possible), in case a mistake occurs.
> It seems that github already supports this behavior.
>
> As of this writing, I see 105 open PR's, 1201 closed PR's for Apache
> Drill.  Perhaps I'm missing something, but why the effort to make this
> automatic?  Are there way more PR's than I'm seeing?
>
> Thanks,
> Dave O
>
> ________________________________________
> From: Timothy Farkas <tfarkas@mapr.com>
> Sent: Thursday, June 7, 2018 1:38 PM
> To: dev@drill.apache.org
> Subject: Re: [Vote] Cleaning Up Old PRs
>
> Hi Dave,
>
> I'm sorry you had a bad experience. We should do better giving timely
> reviews moving forward. I think there are some ways we can protect PRs from
> unresponsive committers while still closing PRs from unresponsive
> contributors. Here are some ideas.
>
>  1. Have an auto responder comment on each new PR after it is opened with
> all the information a contributor needs to be successful along with all the
> information about how PRs are autoclosed and what to do to keep the PR
> alive. Also encourage the contributor to spam us until we do a review in
> this message.
>
>  2. Auto labeling fresh PRs with a "needs-first-review" label (or
> something like that). PRs with this label are exempt from the auto closing
> process and the label will only be removed after a committer has looked at
> the PR and done a first round of review. This can protect a PR that had
> never been reviewed from being closed.
>  3. Allow the contributor to request a "pending" label to be placed on
> their PR. This label would make their PR permanently immune to auto closing
> even after a first round of review has been completed and the
> "needs-first-review" label has been removed.
>
> How do you feel about these protections? Do you think they would be
> sufficient? If not, do you have any alternative ideas to help improve the
> process?
>
> As a note, I think our motivations are the same. We both want quality PRs
> to make it into Drill. I want to do it by removing PRs where the
> contributor is unresponsive so committers can better focus on the PRs that
> need attention. And I think you are rightfully concerned about false
> positives when automating this process. Hopefully we can find a good middle
> ground that everyone can be happy with.
>
> Thanks,
> Tim
>
> ________________________________
> From: Dave Oshinsky <doshinsky@commvault.com>
> Sent: Wednesday, June 6, 2018 6:28:39 PM
> To: dev@drill.apache.org
> Subject: Re: [Vote] Cleaning Up Old PRs
>
> Tim,
> It's too restrictive, unless something can be done to educate (outsider)
> PR authors like myself to "go against the grain" and keep asking.  And
> asking.  And asking.  And asking.  You get the picture?  I did all that.
> And it was ignored.  I assumed that people outside MapR aren't welcome to
> contribute, and/or there was little interest in making decimal work
> properly, and/or there was simply nobody available to review it (what I was
> most comfortable believing), and/or my emails smelled really bad (kidding
> on the last one 8-).  I asked a few times, and asked again a few times a
> few months later, and nothing.  What can you do to educate outsiders as to
> what they need to do to make sure a useful PR doesn't get flushed down the
> toilet?  I spent days learning some amount of Drill internals and
> implementing VARDECIMAL (over 70 source files changed), and did it again
> months later to merge to then current master tip.  All ignored for quite
> some time.
>
> Thanks to Volodymyr Vysotskyi for ultimately grabbing the ball and running
> with it.  That complex a change required an "insider" to bring it fully to
> fruition.  But if the PR had been automatically flushed, I have my doubts
> as to whether the story would have ended the same way.
>
> Thanks,
> Dave Oshinsky
>
> ________________________________________
> From: Timothy Farkas <tfarkas@mapr.com>
> Sent: Wednesday, June 6, 2018 7:07 PM
> To: dev@drill.apache.org
> Subject: Re: [Vote] Cleaning Up Old PRs
>
> Good point Dave. With this automation and a stale period of 3 months, PRs
> would be closed after 3 months of inactivity. However, if you just post one
> comment asking a reviewer to review once every three months, it will stay
> alive indefinitely. Also if you don't want to do this you could request
> your PR to be marked as pending, and it would be exempt from the rule and
> never be closed automatically.
>
>
> The idea behind this automation is to distinguish PRs from contributors
> who are actively working on their PRs and contributors who open a PR but
> then never follow up. In open source, the latter happens often and it
> really overloads the system with PRs that will never be finished. Also
> having this automation with an explicit time limit incentivizes the
> contributor to make noise and comment on the PR to get a review.
>
> In my opinion this is exactly what we want, if your PR doesn't get
> reviewed you should make noise and spam us with messages until we make it
> happen. As long as you keep making noise, your PR won't be closed, and it
> helps keep us honest by doing timely reviews.
>
> What are your thoughts? Do you still feel this is too restrictive?
>
> Thanks,
> Tim
>
>
>
> ________________________________
> From: Dave Oshinsky <doshinsky@commvault.com>
> Sent: Wednesday, June 6, 2018 3:50:15 PM
> To: dev@drill.apache.org
> Subject: Re: [Vote] Cleaning Up Old PRs
>
> Tim,
> It took well over one year before anyone started looking at my August 2016
> PR to implement VARDECIMAL decimal types improvements:
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.
> com_apache_drill_pull_570&d=DwIFAw&c=cskdkSMqhcnjZxdQVpwTXg&r=
> 4eQVr8zB8ZBff-yxTimdOQ&m=MWi8kb0OAU2j5LMIUIewh8w-DPsI0o1XrKdc4X1s9d8&s=
> VRLzr69rpmak_g_UzdY7WYp-qS8QUnsHc7ySiWfzVFE&e=
>
> Volodymyr Vysotskyi ultimately grabbed the decimal types ball and ran with
> it, but I am concerned that my PR and some others would have gotten flushed
> prematurely with this kind of automatic cleaning regimen.
>
> Just my 2.5 cents.
>
> Dave Oshinsky
>
> ________________________________________
> From: Timothy Farkas <tfarkas@mapr.com>
> Sent: Wednesday, June 6, 2018 6:12 PM
> To: dev@drill.apache.org
> Subject: [Vote] Cleaning Up Old PRs
>
> The subject of this vote is whether / how to use probot stale.
>
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__github.
> com_probot_stale&d=DwIFAw&c=cskdkSMqhcnjZxdQVpwTXg&r=
> 4eQVr8zB8ZBff-yxTimdOQ&m=MWi8kb0OAU2j5LMIUIewh8w-DPsI0o1XrKdc4X1s9d8&s=b-
> 1khYEQPqc40pOYraMy-Dw3iGswgnIUXAkHE8YjGEw&e=
>
>
> Please fill out the survey below.
>
>
> https://urldefense.proofpoint.com/v2/url?u=https-3A__www.
> surveymonkey.com_r_NGDCX8R&d=DwIFAw&c=cskdkSMqhcnjZxdQVpwTXg&r=
> 4eQVr8zB8ZBff-yxTimdOQ&m=MWi8kb0OAU2j5LMIUIewh8w-DPsI0o1XrKdc4X1s9d8&s=
> MNgiBpVkL2b8h4VWBYtgclKclzT2p1skDOOu-GeoWhk&e=
>
>
> If you feel this completely misses the mark of what should be done, please
> discuss on this thread. Also this is my first survey monkey poll, so if
> there are any issues please let me know. I'll follow up in two weeks to
> discuss the results.
>
> Thanks,
> Tim
> ***************************Legal Disclaimer***************************
> "This communication may contain confidential and privileged material for
> the
> sole use of the intended recipient. Any unauthorized review, use or
> distribution
> by others is strictly prohibited. If you have received the message by
> mistake,
> please advise the sender by reply email and delete the message. Thank you."
> **********************************************************************
>
> ***************************Legal Disclaimer***************************
> "This communication may contain confidential and privileged material for
> the
> sole use of the intended recipient. Any unauthorized review, use or
> distribution
> by others is strictly prohibited. If you have received the message by
> mistake,
> please advise the sender by reply email and delete the message. Thank you."
> **********************************************************************
>
> ***************************Legal Disclaimer***************************
> "This communication may contain confidential and privileged material for
> the
> sole use of the intended recipient. Any unauthorized review, use or
> distribution
> by others is strictly prohibited. If you have received the message by
> mistake,
> please advise the sender by reply email and delete the message. Thank you."
> **********************************************************************
>
>
***************************Legal Disclaimer***************************
"This communication may contain confidential and privileged material for the
sole use of the intended recipient. Any unauthorized review, use or distribution
by others is strictly prohibited. If you have received the message by mistake,
please advise the sender by reply email and delete the message. Thank you."
**********************************************************************


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message