Re: [DISCUSS] Pull Request review and approval policy

Henry Saputra <henry....@...>

I am +1 for the suggestions to augment with RTC plus lazy consensus

I am sorry for lag of responses for the proposal.

- Henry

On Mon, Aug 27, 2018 at 7:55 AM Jason Plurad <plu...@...> wrote:
There hasn't been any negative responses on this thread. I will submit a PR today to update the PR docs to outline these updates in policy.

1. CTR is already part of our policy
  a. Committers are trusted to decide when a PR can be Commit-Then-Review (CTR) vs Review-Then-Commit (RTC)
  b. Include merging PRs upstream as one of the CTR scenarios
  c. GitHub will not be used to enforce the policy (disable approval reviews) because it does not allow committers to approve their own PRs

2. Our RTC policy requires either
  a. 2 approval votes from committers
  b. 1 approval vote from a committer followed by a one week review period
  c. Committers are trusted to seek 2nd approval for complex changes that require more review

On Friday, August 24, 2018 at 11:35:51 AM UTC-4, Alexandr Porunov wrote:
Will there be a new review and approval policy?
I believe it should be implemented.

+1 for all discussed above.

On Thursday, August 9, 2018 at 7:57:16 PM UTC+3, Jason Plurad wrote:
Perhaps a weekly update on the PR queue posted on the dev mailing list would be a good idea.

Something like:

# 0.2.2 queue
PR a - needs one more approval
PR b - waiting on revisions
PR c - unreviewed for 16 days
PR d - new this week

On Thursday, August 9, 2018 at 1:17:40 AM UTC-4, Jerry He wrote:
Finding the right balance between more review/more approvals and being more efficient is what we aim for.  I have been in situations in my PRs where another pair of eyes caught some error I made.  I was also in situations where my PRs sitting there for a long time waiting for reviews.

I think I am ok with adopting 2-4 from the TinkerPop process. 
But adding some extra guard (e.g an extra reminder or ping before commit) or time (more time than a week to accommodate busy professionals?)  will make it easier to relieve concerns.



On Wednesday, August 8, 2018 at 1:14:06 PM UTC-7, Alexandr Porunov wrote:
I am not a committer but here are my thoughts:

1. I think it is enough to maintain 2 active branches right now. Latest release + future release (master). There is plainly no enough committers to maintain more active branches.

2. I think it is worth adding steps 2-4 because there are some PRs with huge delays.

On Wednesday, August 8, 2018 at 6:25:49 AM UTC+3, Jason Plurad wrote:
I have a couple discussion topics to bring up based on our recent history of getting releases out.

1. Based on what we have written in the docs (Chapter 44. Pull Requests), we have a commit-then-review (CTR) policy, however it does not work in practice. GitHub does not allow a committer to approve their own PRs. This makes it impossible to merge PRs without at least 1 other approval, and this has caused delays on some PRs to get merged. For example, the master branch is not yet open for business because the PR is still waiting for approval. Another situation was when we needed to merge a fix from 0.2 upstream into master. The 0.2 PR got approved and merged, but either the committer didn't open a separate PR to merge to master or the committer opened the PR to merge to master but that PR stagnated on getting approval. If we end up in a situation where we are maintaining more than 2 active branches, there would be even more friction involved getting changes merged upstream.

Based on these docs, we can disable restriction checks for repository administrators. If we still want to allow CTR, it seems like this needs to be done and all committers would need to be in a repository administrator group.

2. Currently our policy is for 2 committer approvals to merge a PR. Toward the end of the release cycle, I had gotten in the habit of performing my review approval first and then requesting reviews from the JanusGraph/committers group via GitHub. This should send a notification to all the committers in the group. I have no idea how other committers manage their GitHub notifications, but it seemed like a better approach than spamming the dev list or backchanneling individuals every time a PR needed a second review.

TinkerPop recently introduced a new approach to their PR review process:
We seem to agree that in the future we will go with the following review-the-commit (RTC) process:

1. Each change to TinkerPop release branches requires 3 +1s from committers and no -1s OR
2. A single +1 from a committer and a 1 week review period for objections (i.e. "cool down period") at which point we will assume a lazy consensus
3. The single +1 may come from the committer who submitted the PR in the first place
4. Committers are trusted with their changes but are expected to request reviews for complex changes as necessary and not rely strictly on lazy consensus
5. commit-the-review (CTR) procedures remain unchanged
By adopting bullets 2-4 from TinkerPop's process, I think it would enable us to merge changes into the codebase more efficiently. If committers were concerned about what PRs might get merged, they would need to check the PR queue at least once a week.


You received this message because you are subscribed to the Google Groups "JanusGraph developers" group.
To unsubscribe from this group and stop receiving emails from it, send an email to janusgr...@....
To view this discussion on the web visit
For more options, visit

Join { to automatically receive all group messages.