Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

doc: leave pull requests open for 72 hours #22275

Closed
wants to merge 1 commit into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 12, 2018

Another attempt to simplify procedures. Again, this is something I want buy-in on and not something I want slipping in unnoticed so: @nodejs/collaborators

Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

The 48/72 rule predates our fast-tracking process. Given the ability to
fast-track trivial pull requests, there should be little disadvantage to
leaving significant changes open for 72 hours instead of 48 hours, and
arguably considerable advantage in terms of allowing people sufficient
time to review things.

So to simplify, standardize on 72 hours. Weekend or not, 72 hours. Easy.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • documentation is changed or added
  • commit message follows commit guidelines

Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

The 48/72 rule predates our fast-tracking process. Given the ability to
fast-track trivial pull requests, there should be little disadvantage to
leaving significant changes open for 72 hours instead of 48 hours, and
arguably considerable advantage in terms of allowing people sufficient
time to review things.

So to simplify, standardize on 72 hours. Weekend or not, 72 hours. Easy.
@Trott Trott added the doc Issues and PRs related to the documentations. label Aug 12, 2018
@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Aug 12, 2018
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unnecessary. -1

Copy link
Member

@trivikr trivikr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@devsnek
Copy link
Member

devsnek commented Aug 12, 2018

i think the times are more nuanced than this. 72 hours on the weekend guarantees it spills into at least 48 hours of weekdays. on the other hand 72 hours from a weekday will end up having people open prs on monday to get them landed quickly so they can focus on other things. to me anyway having more than two prs open is stressful.

Copy link
Member

@devsnek devsnek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Trott
Copy link
Member Author

Trott commented Aug 12, 2018

72 hours on the weekend guarantees it spills into at least 48 hours of weekdays.

Current rules guarantee only around 24 hours of weekdays if a pull request is opened early on a Saturday.

72 hours from a weekday will end up having people open prs on monday to get them landed quickly

This proposal isn't 72 hours from a weekday. It's just 72 hours, no matter what day.

This change should stop the kind of gaming described. If it's 72 hours from whenever you open it, then it makes sense to open it as soon as it's ready. Under current rules, if you have something ready to go on a Sunday afternoon or evening, then it is conceivably better to wait until Monday morning to open the pull request if you want it to land quickly.

@Trott
Copy link
Member Author

Trott commented Aug 12, 2018

By the way: The weekend isn't even over yet and we've already had four non-fast-tracked pull requests land with less than 72 hours of time open.

We can debate whether that's really a problem or not. (If a pull request is opened on Thursday morning, is 62 hours enough? Strict interpretation of the rules would say "no" in my opinion, but strict interpretation and reasonable interpretation are often not the same thing. However, this proposal would eliminate that ambiguity.)

Simplifying the rule to always be 72 hours would probably result in more compliance since there would be less confusion and less ambiguity.

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current rules are unclear re: weekends, and I apparently interpreted them differently from e.g. @Trott — I supposed that if a PR had a 48-h non-weekends window, then it was good to go, no matter when it was opened.

Also the current rules are hard to interpret because of the timezone differences, and the only way to ensure that everything is fine was to wait more.

This change simplifies things.

Copy link
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way: The weekend isn't even over yet and we've already had four non-fast-tracked pull requests land with less than 72 hours of time open.

This convinced me.

@devsnek devsnek dismissed their stale review August 12, 2018 08:03

i'm kinda -0 at this point.. i'm hoping with more tooling can eventually get us to a world with the only rule being "every pr must be open for at least 48 hours of utc weekday time"

@mcollina
Copy link
Member

I agree that the rules are confusing. I've always interpreted this as "cannot land anything that had been open less than 72 hours on the weekend or Mondays". So, if I wanted to land something on Monday, I had to open the PR on Friday.

I don't think that waiting 72 hours is beneficial for the project, as it will just increase the chance of rebasing. It would be good to have some data to back this one. How many PRs are open for less than 72 hours? How many have caused problems?

This will have a net effect of slowing down the velocity of the project. Is this a goal for all people that LGTM this?

@benjamingr
Copy link
Member

This will have a net effect of slowing down the velocity of the project. Is this a goal for all people that LGTM this?

I don't think it will - I approved this since Rich noted we had a lot of stuff that landed after less than 72h anyway so the rules weren't being followed. I always read 72h as "letting people who are off for the weekend have a say".

Most pull requests that introduce significant changes usually take more than 72h to figure out anyway since there is actually some back-and-forth discussion. In addition, a bunch of PRs that need 48h take more than 72h in practice (especially if it's not the author who's landing them).

Can you give an example of one or two pull requests that would take longer with a mandatory 72h wait (that fast-tracking does not resolve)?

@mcollina
Copy link
Member

Can you give an example of one or two pull requests that would take longer with a mandatory 72h wait (that fast-tracking does not resolve)?

I have landed several PRs with a 48hours timeframe, specifically before releases. I'll dig those up, but I'm on vacation for the rest of the week, it might take some time.
I'd expect that everything that took < 72 hours to land would at least increase to 80-90. This increases the chances of conflicts, rebases, and repeated CI runs. This will increase the number of "in-flight" pull requests and they will require more work to land them, specifically because a lot of people asks for another LGTM after a rebase. All of this will slow down velocity during normal work days.

@benjamingr
Copy link
Member

I'd expect that everything that took < 72 hours to land would at least increase to 80-90.

I'd expect that 95% of things that took < 72 hours to land would get fast-tracked so I'm specifically looking for examples of things that would not get fast-tracked.

@vsemozhetbyt vsemozhetbyt added the meta Issues and PRs related to the general management of the project. label Aug 12, 2018
@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Perhaps this: make it a consistent 48 hrs for all changes, rather than 72. It achieves the consistency goal, does not impact velocity, and is a better match to current practice. The 72 hr rule is the special case, so let's eliminate it.

Do that, and provide for a 7 day escape hatch for the minimal 2 sign offs proposal and I'll drop my objections to both.

@benjamingr
Copy link
Member

@jasnell do you have some examples of PRs where leaving them open for less than 72 hours was beneficial for the velocity of the project and would not get fast-tracked?

@jasnell
Copy link
Member

jasnell commented Aug 12, 2018

Nope, but then again I'm not the one proposing that a change is necessary here.

I have absolutely no concerns about project velocity under the current rules, but can be sympathetic to the notion that the current rule can be ambiguous so eliminating the special case is worthwhile.

@benjamingr
Copy link
Member

Nope, but then again I'm not the one proposing that a change is necessary here.

To clarify: I did not try to say your opinion isn't valid or that you're wrong - I just read your comment and wanted some examples since I was interested in the argument you were making. I certainly wouldn't want us to make this call without sufficient data.

At the end of the day I'm less concerned with us making this or that particular change and more concerned with coming up with a better policy overall.

At the moment you are the only one objecting to this PR which is why I think it is important for your opinion to be heard and any concerns you have to be addressed before we make this change if at all possible (and if the change is made).

Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, but just want to add that I'm worried that since additional review time would be required, people may start to fast track larger and larger changes. One answer could be a minimum amount of time a pull request (fast tracked or not) needs to be open. 12 hours? 24 hours?

@maclover7
Copy link
Contributor

Also, thoughts about using required GitHub status lights (via the github-bot) to enforce these policies, so it's less mental overhead/keep track of?

Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm indifferent to picking 48 or 72 hours, but this rule has confused me many times, so +1 to getting rid of it.

@benjamingr
Copy link
Member

I'm indifferent to picking 48 or 72 hours, but this rule has confused me many times, so +1 to getting rid of it.

The problem with 48h is that a bunch of people work on Node.js as their daytime job. Those people (as well as others that simply have that time preference) are not available over the weekend for code reviews.

While I personally don't fall into that camp I'd like Node.js to cater to those people. Some people have a deep caring of certain subsystems and landing significant changes without giving them a chance to see the code first is something I'd prefer to avoid.

Of course fast-track PRs don't fall into this category :)

@ofrobots
Copy link
Contributor

ofrobots commented Oct 4, 2018 via email

@rubys
Copy link
Member

rubys commented Oct 4, 2018

@rubys What is the most active project where you do that? Let's define "most active" as having the highest velocity (commits per day) and the most individuals with commit access.

Sorry, I missed this question. Nearly every project at the Apache Software Foundation is Commit then Review. My first software project was PHP, and it also ran this way. I'm not a collaborator on Rails, but it also runs this way.

I abstain.

@jasnell
Copy link
Member

jasnell commented Oct 4, 2018

The 48/72 hour rule is generally unique to Node.js. I know of no other major open source project outside of the Node.js ecosystem that uses anything similar. As one of the people who originally argued in favor of the rule, it's existence predates LTS, predates our current release cycle, existed at a time when CI was nearly always red, and was added when there was still significant lack of trust among contributors following the io.js fork. Given where we are now, and given that the two review rule has gone into effect, I'd much rather we go in the opposite direction on this and move towards eliminating the time rule entirely. Failing that, moving to 48 hours addresses the consistency issue.

@Trott
Copy link
Member Author

Trott commented Oct 4, 2018

eliminating the time rule entirely

@jasnell FWIW, I wouldn't take that off the table either. I said above that the plan was to start with this rule, and to next see how it fares against the more liberal 48-hour rule. If the 48-hour rule is deemed more desirable than this, I have no problem with then seeing how 48-hour rule fares against no time-based rules whatsoever.

@mcollina
Copy link
Member

mcollina commented Oct 4, 2018

I’m -1.

@MylesBorins
Copy link
Contributor

+1

@addaleax
Copy link
Member

addaleax commented Oct 4, 2018

To be clear, my “yes” vote hear is because I prefer a consistent time, and I will vote for 48 hours rather than 72 in the subsequent vote.

@cjihrig
Copy link
Contributor

cjihrig commented Oct 4, 2018

To be clear, my “yes” vote hear is because I prefer a consistent time, and I will vote for 48 hours rather than 72 in the subsequent vote.

Same here.

@TimothyGu
Copy link
Member

Agreed with @addaleax.

@Trott
Copy link
Member Author

Trott commented Oct 4, 2018

10 YES, 3 abstentions, 2 NO, 4 TSC folks not yet voting/abstaining. This passes. Thanks everyone. Now on to consider 48 hours. Please stand by...

Trott added a commit to Trott/io.js that referenced this pull request Oct 4, 2018
Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

The 48/72 rule predates our fast-tracking process. Given the ability to
fast-track trivial pull requests, there should be little disadvantage to
leaving significant changes open for 72 hours instead of 48 hours, and
arguably considerable advantage in terms of allowing people sufficient
time to review things.

So to simplify, standardize on 72 hours. Weekend or not, 72 hours. Easy.

PR-URL: nodejs#22275
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@Trott
Copy link
Member Author

Trott commented Oct 4, 2018

Landed in 2f117e1

@Trott Trott closed this Oct 4, 2018
targos pushed a commit that referenced this pull request Oct 5, 2018
Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

The 48/72 rule predates our fast-tracking process. Given the ability to
fast-track trivial pull requests, there should be little disadvantage to
leaving significant changes open for 72 hours instead of 48 hours, and
arguably considerable advantage in terms of allowing people sufficient
time to review things.

So to simplify, standardize on 72 hours. Weekend or not, 72 hours. Easy.

PR-URL: #22275
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
targos pushed a commit that referenced this pull request Oct 7, 2018
Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

The 48/72 rule predates our fast-tracking process. Given the ability to
fast-track trivial pull requests, there should be little disadvantage to
leaving significant changes open for 72 hours instead of 48 hours, and
arguably considerable advantage in terms of allowing people sufficient
time to review things.

So to simplify, standardize on 72 hours. Weekend or not, 72 hours. Easy.

PR-URL: #22275
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
jasnell pushed a commit that referenced this pull request Oct 17, 2018
Currently, we have a 48/72 rule for how many hours a pull request should
be left open at a minimum. Unfortunately, whether a pull request should
be left open for 48 or 72 hours is often unclear. The 72 hours is
required if it is a weekend. If I open a pull request on a Friday
morning, does it need to stay open 48 hours or 72 or something in
between? Does it matter if I'm in one time zone or another?

The 48/72 rule predates our fast-tracking process. Given the ability to
fast-track trivial pull requests, there should be little disadvantage to
leaving significant changes open for 72 hours instead of 48 hours, and
arguably considerable advantage in terms of allowing people sufficient
time to review things.

So to simplify, standardize on 72 hours. Weekend or not, 72 hours. Easy.

PR-URL: #22275
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Reviewed-By: Trivikram Kamat <trivikr.dev@gmail.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Jon Moss <me@jonathanmoss.me>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Ben Coe <bencoe@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: João Reis <reis@janeasystems.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@Trott Trott deleted the 72 branch January 13, 2022 22:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. meta Issues and PRs related to the general management of the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.