drill-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dave Oshinsky <doshin...@commvault.com>
Subject Re: [Vote] Cleaning Up Old PRs
Date Thu, 07 Jun 2018 18:34:13 GMT
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."
**********************************************************************


Mime
View raw message