HN Explorer

Ask HN: How are pull requests integrated in a repo with high commit frequency?

118 points by dist1ll ↗ HN
So let's assume we open a pull request, merged current main into our feature branch and are running tests. The full acceptance test suite takes 5 minutes to complete. Meanwhile, in those 5 minutes, 10 other commits have been made. If we merge now, we can't be sure that we won't break stuff right?

So after merging the pull request, are you supposed to perform another set of regression tests? Hard to find info about that

70 comments

[ 3.8 ms ] story [ 141 ms ] thread
Revell
Sounds like you want to implement something like merge trains?[0]

[0]: https://about.gitlab.com/blog/2020/01/30/all-aboard-merge-tr...

chrisshroba
This is a great solution and while I was at Google I noticed several high commit frequency teams using this strategy. Of course Google had built a bunch of custom tooling and infrastructure around it, so I can’t vouch for how easy it would be to integrate into a different company’s dev workflow, but if there are enough developers to make it worthwhile, then tasking a few developers with setting this up should be a useful allocation of engineer time.
vasco
Merge trains are the way. They do optimistic testing of the prior branches in the queue so the happy path goes fast.
carlmr
Another question I got on this. What do you do with merge conflicts between the branches? Do you then provide the system with patches for the merge conflicts somehow, so that it can resolve them as needed?

E.g. when merging A, B and C. C might have no conflict with master or A but it has a conflict with B. Now for the queuing to work A -> B -> C it will have to know somehow how to patch C to fit on top of B.

Maybe B breaks if it is not working after merged into A, and then the queue becomes A -> C -> B (modified), now B probably needs a patch to merge cleanly on C?

Communitivity
The developer whose task branch had merge conflicts was responsible for resolving the merge conflicts, and doing sidebars as needed where a discussion of the approach was warranted. They would resolve those conflicts in their branch before the PR was submitted. If the conflicts happened between submission and approval then the conflicts would still be resolved on the task branch before merge.
carlmr
>The developer whose task branch had merge conflicts was responsible for resolving the merge conflicts

But that's exactly my point. The first PR to merge cleanly will determine whether the next PR causes a merge conflict or not. At the time of merging, none of these PRs had a merge conflict.

We often have 10 conflicts between PR admission and approval.

avisser
> We often have 10 conflicts between PR admission and approval.

I don't think a devops solution can remove those conflicts. At least not all of them.

If you have devs in the same hot-spots of the code, you're going to get conflicts. Maybe refactoring can address some of this core issue.

deckard1
The key is to reduce the number of developers touching a specific section of code, and line up all tickets and hand them over to a single developer (or two).

And yeah, management won't do it. Fred Brooks wrote an entire book about this in 1975 that no one reads today and everyone would certainly ignore if they did read it. Because it tells you the unvarnished truth about the nature of communication and information flow within an organization. Such is the state of things in our industry. Sweet little lies.

mathstuf
We call ours the "stage". Merge conflicts are just a "please wait" signal. The way it works is:

  - the stage is always up-to-date with the target branch (so if it updates, we re-merge all topics on top of the new target branch head)
  - topics should pass CI standalone before being staged
  - the stage is first-come first-served, so "later" topics have to wait
  - updating a topic puts it at the end of the line
  - topics should really go through the stage before landing in the target branch
Conflicts (content or logical) will cause a topic to have to wait until the conflicting topic has landed in the target branch before it can participate in the stage. This usually only affects topics working in the same area.

Our library implementing this: https://gitlab.kitware.com/utils/rust-git-topic-stage

However, given your later metric of 10+ conflicts per topic…I suspect your project is still in the "getting off the ground" phase where a stage is awfully heavy process because there aren't "bright line" distinct sections of the code yet. Or your topics are too big. Hard to say.

[deleted]
(comment deleted)
dmurray
You run the tests on the same snapshot of the code you are about to deploy. That might be 10 minutes or 2 hours or a month behind your trunk branch that got the 10 new commits.
apetresc
Check out something like Bors, which implements a paradigm that fixes this class of issue: https://github.com/bors-ng/bors-ng

(Homepage: https://bors.tech/)

dathinab
I can recommend it.

Even if you don't have a supper high commit frequency it is still nice to enable certain rebase/stacked PR workflows. Which can remove a lot of friction even for teams which "just" have idk. 6 PRs merged per day (but other PRs which are based on top of this PRs in review and progress maybe stacked multiple layers deep).

mijoharas
Does bors stand for something? what does the ng stand for? (I wasn't able to find out while googling.)
pxeger1
-ng is a suffix sometimes used for "reboots" of abandoned or poorly maintained software; I believe it stands for "next generation".
quinncom
Thanks, I assumed that's what it meant. It's not terribly common, but there are a few packages using this naming convention: aircrack-ng, bwm-ng, crosstool-ng, libcap-ng, lincity-ng, minizip-ng, proxychains-ng, raxml-ng, recon-ng, stress-ng, zlib-ng, macfusion-ng, shadowsocksx-ng
rrss
does bors limit the commit frequency / PR merge frequency to one merge every test runtime (5 minutes in OP), or does it do something more complex and speculate / run some stuff in parallel?

OP’s numbers are 120 commits per hour and 5 minute test run, I’m wondering if adopting bors would necessarily reduce that to 12 commits per hour.

henryfjordan
Bors batches PRs, tests the batch, then merges the whole batch at once. It's made to handle high-throughput repos.
krab
It tries to build all queued changes together at first. So if you don't have any conflicts, there is no throughput limit.
anelson
We use bors as well. Our repo isn’t particularly active, a few dozen committers at any one time, but it lets us have pretty aggressive checks on master without having to run them separately for each PR before it lands.

Definitely recommended. We run ours on a Heroku instance it’s cheap and almost entirely maintenance free.

__alexs
Such a code base is likely to be large and the changes distributed somewhat uniformly across it. This means that conflicts are actually not super likely and also that your assumptions are not highly likely to get invalidated by the commits that happen in between.

If your change does break after merging, it can usually either be quickly fixed with a minor code change or simply reverted immediately.

andersco
Is it possible that the solution is to look at why there is such a high frequency of commits by different devs into the same main branch?

Rather than look for a technical solution I would explore process and culture changes, such as having the various devs making all the different commits start pairing.

brianwawok
Sounds like a good way to cut productivity in half!
silon42
Or split the project into multiple modules/services?
regularfry
High frequency of commits into main (assuming they're also release candidates) is what we would term a good problem to have.
sandspit
There are two great saas solutions for this - mergify.io and mergequeue.com.
sz4kerto
Our full acceptance test run in 3 hrs (on build servers with 64 cores and 3xx GB memory) and we have PRs coming up every hour or so.

We run the acceptance tests for all PRs and also the main branch after each change. So a given PR needs to be 'green', and if we merge the PR then we run the tests on the integration branch too. If the integration branchs gets broken somehow, then all merges stop -- we don't merge any further PRs into a 'broken' branch.

We have tried the option of always rebasing PRs on the most recent integration branch and re-running the tests, but that results in exponential number of builds.

Of course you need a meaningful build farm -- we have around 1000 CPU cores for ~30 developers that are fully utilised during work hours.

Jyaif
1000 CPU for 30 developers sounds enormous. What kind of software are you working on?
sz4kerto
Patient portal + electronic health records.

1000 CPU cores, not CPUs. :)

Cerium
I work on a project with a 3 hour CI process that handles about 15 to 40 PRs a day. The problem you describe here is a real challenge. Our PRs don't use a merge button. Once CI passes (your changes built on latest develop), then you can click a box that says this PR should merge. An automated process scoops up all those PRs each night and builds them together. The system test team tests that build in the morning. If there is a problem, that is debugged (we build build some bifurcation builds ahead of time to make this faster). Then the batch is merged to develop. This process has worked very well for us. Develop keeps a good pace but is only very rarely broken.
drewcoo
What are your "regression tests" and why are you only worried about those?
hnrodey
Run the tests before allowing the pull request to complete?
epage
While tools like Github check for patch conflicts, they don't check for semantic conflicts (e.g. one PR adds a new use of a function another PR removes).

This is where merge queues come into play which have a variety of names (merge queue, submit queue, merge train, etc)

I have a write up on merge queues [0] that goes into other benefits (e.g. speeding up PR builds), design considerations and their trade offs, and a comparison of various implementations.

[0] https://epage.github.io/dev/submit-queue/

jakub_g
This is a real problem in my current place (huge monorepo, 100+ devs, 100+ merges per day).

Like others said, "merge queue" is the solution. GitHub's one has been in beta for many months now.

There are also dedicated companies doing this like https://mergify.com/

But there's several tricky aspects like "what if I want some commit to jump in front of queue?", compliance etc.

Due to the multitude of requirements and off-the-shelf solutions being somehow limited, infra folks in my place are considering building a custom solution :)

jd__
I'd be curious to know what you find limited in off-the-shelf solutions?

(disclaimer: I'm from the Mergify team )

lhorie
One question and one comment.

Question: You list Uber (my employer) as a user in your homepage, but searching internally, the only search result for mergify is a mergify.yml in some random debian module for lua, so it doesn't seem like we actually use it for anything (and we built an in-house solution for the repos w/ serious commit traffic). I'm curious how you determine who your corporate users are?

Comment: we ended up building a merge queue in house because frankly build/test pipelines can look very different,(e.g. at one point we had jenkins pipelines and buildkite pipelines, some using bazel, some not, talking to phabricator instead of github, etc). At the end of day plugging in a merge queue technology requires a bunch of work integrating with a myriad of things anyways. Since the engineering effort is relatively high regardless, doing it in house lets us experiment with more aggressive optimization heuristics than just waiting on a 3rd party.

jd__
We know Uber has some great internal tooling (there's even a paper published on their merge queue: https://eng.uber.com/research/keeping-master-green-at-scale/). We had various Uber engineers using Mergify on open source repositories those last years. I didn't check recently if that was still the case, I'll make sure our lis is not too much outdated.

I definitely agree with your view on how pipelines can be different. I think none of our customers has something that is exactly the same. However, many of them don't have the workforce to build in house or even to optimize as far as you would do in a (very) large company.

lhorie
Ah, good point, our internal search tool only indexes internal code, so that makes sense, thanks!
readme
i would perform the tests before merging - rebase the pull request branch on top of master and run the tests, then merge using the --no-ff flag to ensure a merge commit is still entered
pavel_lishin
With a sufficient number of merges, by the time your rebased branch's tests have finished running, your branch is no longer up to date with the main branch - so you get to rebase again, wait for tests again, etc.
regularfry
It sounds like what that's missing is a while loop and a coffee break.
efxhoy
We run tests on the master branch before we deploy. If tests fail there we find the offending PR and revert it.
Communitivity
I've worked on two projects in the last six years that used PRs and had a high commit frequency.

Two things had to work or we would have lost all our sanity points (reference to Call of Chthulhu): automated checks of style, building, testing; and small PRs subscribing to the 'do one thing and do it well' approach.

As for the second, it's more doable than you might think - if you decomposed your work well. We decomposed our work at these levels on one project: product, demo capability, epic, task; the other project used feature, epic, task. Typically PRs were at the task level, though sometimes at PR level.

For us this had the added benefit that the developer velocity on these two projects exceeded the velocity of any of the other projects I've been on.

Some other tricks we used were to make sure we rebased our task branches from develop every morning (and if needed after lunch); each task branch had 1 person only working on it; where complexity warranted we created an Epic-### branch for that epic and treated it as a mini-develop for tasks on that epic.

scottmsul
Wait, is it 10 other commits, or 10 other pull requests? For medium-sized projects (5-10 devs) it's generally good practice for devs to make a separate branch per feature, make lots of commits on the side branch, then merge the side branch into master in a single PR once the feature is ready.
Aeolun
We don’t try to deal with that issue. If something breaks because of the merges, it’ll be the next persons problem (when they make a new feature branch off of master and have a few failing tests).

Our test suite takes about 30m to finish.

lhorie
The technology you're looking for goes by many names, "submit queue", "merge queue", "merge train", etc.

In a nutshell, the idea is to keep track of currently running jobs, then any time a new commit enters the queue, you merge all the running commits into that and test that as a bundle. If merging fails, bail out. If that bundle job fails, bail out. If one of the previously running commits fail, you bail out of the bundle job and run another speculative bundle job without the failed commit. When a bundle succeeds, land all of its commits and abort any remaining redundant jobs.

Such systems will sometimes have heuristics to bundle commits together in some smarter way than a naive queue (e.g. preferring to bundle of commits without overlapping changes, or not bundling small commits with huge ones to prevent infecting speculative builds with the slowness of the big commit, using AI to come up with heuristics to detect "likely to fail" commits and preemptively starting speculative builds without them, etc)

dist1ll
That's amazing, I haven't heard of these terms before. I looked through some of the solutions posted here (GitLab Merge Trains, mergify, ZUUL Gate) and they are awesome.

I just felt that merging into main without having 100% confidence that acceptance tests would pass means that true continuous delivery isn't achieved. But with these tools, we can make sure that our software is always in a releasable state (according to our test suite). I learned a lot today.

jFriedensreich
we rebase all branches on top of the target branch and only allow fast forward merges, which prevents any untested merge commits to cause issues and is really good for reading history. if there are changes to the target branch since the last rebase, there needs to be another rebase triggering automatic CI/CD. if there is a known bottleneck where this would happen a lot to a number of PRs and all the prs are green we rebase the prs on each other in the desired merge order just have to wait for the youngest PR to run through ci/cd and then can fast forward merge all at once.
msk20
There is ZUUL Gating[0] CI it is actually the perfect solution for this. It works with Github or Git based repository system.

It automatically tests the changes with a simulated merge on master together. So it orders PR1 -> PR2 -> PR3 -> .... -> PR-100 by order of approval. If PR1 -> PR2 (Fails) -> PR3 -> .... -> PR-100

It restarts -> PR3 -> .... -> PR-100 and Up after removing PR2. This behavior is even customizable.

Video of it in action: https://zuul-ci.org/media/simulation.webm

Links: [0]:https://zuul-ci.org/

mountainriver
Kubernetes does this with a tool called Prow, but it’s roughly what others have mentioned around merge trains
yakubin
At dayjob we have a "commit queue" server. When a patch is accepted on Gerrit, there is a button to send it to the queue. The queue cherry-picks the commit onto the current master branch, makes several builds (different configs) and runs tests. If everything is ok, it pushes the commit to master and marks the patch on Gerrit as merged. If any of the builds or tests fail, it doesn't push the change and instead makes a comment on Gerrit with a link to the broken build/test.

So all commits are made sequentially, but most of the time developers don't need to rebase them themselves before pushing. The time distribution of commits sent to the queue is non-uniform, so you may have a long queue during the day, but by late evening it's pretty much empty. It has the greatest number of commits on Fridays.