Several developers have recently voiced their frustration at the amount of time it takes to get a PR merged. This issue is especially visible in Foreman core, where we have been standing at around 120 open prs for quite a while, but is also true for many of the other repos in the Foreman and Katello organizations. The purpose of this (rather long) post is to try and identify some possible causes for this and start a discussion on what steps we can consider to improve the situation.
I think it goes without saying that getting PRs merged faster will lead to better developer happiness and productivity, and value gained by our users by having features added and issues resolved quickly. For first time contributors, getting their contribution accepted quickly is a very important step to becoming returning contributors.
Some of the causes I’ve identified so far from what I’ve seen, feel free to add more if I missed something:
- Foreman core team has 14 members. If we look at the committer statistics, we see that in the past two years, 76% of the commits were merged by just 5 of these, three of which don’t currently work on Foreman full time. As some people have moved on or taken additional responsibilities over the last few years, we have lost many core committers, with very few people who have stepped in to take this responsibility (we added @mmoll and @Shimon_Shtein in Dec '17 and @ekohl in July '18). In the same time frame, the number of developers working full or part time on foreman has grown significantly, producing more new PRs all the time.
- This is also true for plugins - the number of plugins and people working on them continues to grow faster than the rate at which new maintainers are added.
- Many of the repos have just one maintainer, who has other responsibilities and can’t review all PRs properly. In these situations, the maintainer also has a very hard time getting their own code reviewed, often resorting to merging their own code with no other eyes on it - sometimes leading to bugs that could have been caught by even a short review.
- Maintainers are very reluctant to reject PRs, instead avoiding reviewing them until eventually the bot closes them out after half a year.
- In other cases, reviewers may decide that the review cycle has dragged for long enough and compromise on quality to get a PR merged, possibly planning to fix up any issues in follow up PRs (which often never get the priority they need to be done), leading to increasing technical debt with no clear plan on how or when to pay it back.
- The longer the review cycle(s) take, the more difficult it is for both reviewers and contributors to get back to the code and remember why certain things were done in a certain way or what was the requested change etc.
- Some PRs touch areas in which no one has expertise (for example, they were written by past contributors who are no longer active), leading to fear of breaking something, or even not understanding the change, by reviewers. These can often be spotted as the PRs that stay open for a long time with no comments on them - as no reviewer feels comfortable enough reviewing them at all.
- Bigger changes are harder to review. A while ago I saw a talk by Greg Kroah-Hartman, where he mentioned that they usually don’t accept patches longer than about 100 line change into the stable kernel. This got me thinking, and I’ve asked @Gwmngilfen to scientifically look into our data regarding the relation between size of the PR and the time-to-merge. He’ll share the results once he’s done, but my hypothesis is that the difficulty of reviewing a PR grows exponentially by the size of the changes.
Now that I’ve outlined some of the causes, and if you’ve made it this far, let’s look at some possible options as to how we can improve the situation:
- Adopting a more liberal merge policy - for example, merging everything that passes tests. I don’t think this option is feasible, considering how critical stability is for our users and how many areas don’t have proper test coverage.
- Encourage more people to do reviews. As mentioned in 1 above, very few people do the majority of reviews. I’ve checked several other larger repos and the situation is similar or even worse. Also, the number of non-committers who do many reviews is very small, even though that makes it easier for committers to actually accept patches (as they had already been reviewed), as well as making it possible for them to become committers eventually. I don’t really have any good idea how to promote this other than constantly reminding people to do it, suggestions are welcome.
- Increase the number of committers in the various repos. To become a committer, we usually require people to do a certain amount of good reviews first. While we could become more lenient with this requirement, I’m concerned it may again lead to lower stability due to less experienced committers. Perhaps we could have some process in which people who are interested in becoming committers go through some sort of mentoring by experienced committers.
- Speeding up reviews by doing them together with the contributor. In many cases, weeks of back-and-forth correspondence can be saved by sitting together (in person or by video) for half an hour and discussing the changes. Any comments raised during this review should be written down in case other reviewers are involved, but this can help the reviewer understand the change much better and save the repeated context switching and possible misunderstandings of written communication. I’ve experimented with doing this several times, and it has led to much faster turnaround and both sides understanding the code better and feeling better about the experience.
- Splitting up larger PRs into smaller, atomic changes. A small PR will always be much easier to understand and review properly. In the past we’ve been reluctant to merge “partial” work, mostly due to the reasonable concern that it may lead to dead code that is never used, and also because in some cases doing a change gradually in several steps requires more code in total and more planning than doing a larger change all at once. For the first point, we could ask for the “next-steps” PRs to be open at the same time, or perhaps even better, not - it might actually save a lot of effort going down a certain path if during review of the first step a better path is found, and in fact save the effort of having to rewrite big parts of a PR following the review. For the second point, while the total coding effort might be bigger, the actual time to delivery would be shorter. Since I believe the time to merge is exponential to the size of the change, five 50-line changes would actually be merged much faster than one 200-line change. I believe this will also lead to less bugs, as many regressions we’ve had were a result of changes that are too large for any single person to properly understand and review them, and were just missed between the hundreds or thousands of lines in a single PR.
- Limit the number of “in the air” prs for developers. This is obviously possible only for people paid to work on Foreman, but I know some teams have experimented with placing certain limits on the number of PRs allowed in waiting for review, trying by that to encourage their members to put more effort into making sure their work is reviewed and merged before they start something new. I’d be happy to hear from those teams how this worked out for them.
- Have specific times (perhaps parts of the release cycle?) dedicated to merging PRs - a week or more where all developers are requested to only work on getting open PRs merged and not create any new ones. I think we’ve had a couple of times where we tried doing such “merge parties” in the past, but never for more than a day AFAIR. Perhaps we could try doing this for a week and see how that goes.
- Be more explicit in closing out PRs that we don’t feel have a possibility of being merged, instead of leaving them open for the bot to close. While this may lead to hard feelings on the contributors’ side, so does having your PR open and ignored for half a year until the bot comtes. It may be better, like a band-aid, to close it quickly rather than spend several cycles of reviews and effort trying to improve it only to eventually let it rot for the bot. If we decide to do this, we need to find a way to do so with the needed sensitivity.
- The above point is also true for issues on our tracker - we don’t close any requests right now, and this leads to many open issues that will never be fixed, or people implementing fixes that perhaps shouldn’t have been implemented at all, just because they are in the tracker.
Wow, this came out much longer than I initially expected, thank you for reading all of this!
If you have any further suggestions or disagree with anything I’ve written above, please add your comments below. Please also reply with what of the above options you think we should go for.
Pinging @core and @katello - since you are doing much of the reviews, your opinions are especially important in this discussion.