Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Successfully Merging the Work of 1000 Developers

engineering.shopify.com

Post reply on HN
Print view View original

61–70 of 108 posts

Re: Successfully Merging the Work of 1000 Developers

#61

Post by hinkley »

hinkley wrote: post #44
tekstar wrote: post #4

Earlier quoted context omitted.

Shopify always runs CI on branches before merging to master. Everything this article describes is in addition to that, in order to deal with the problems the article talks about at "merge to master" time, like 2 merged PRs failing or a stale PR that passed on branch but fails on master due changes. At this scale you need to be deploying constantly, otherwise deploys are hundreds of commits large and its impossible to…

It smells like a capacity planning error. What's the minimum residency time to reliably detect problems with my PR? Add deployment time, double to account for jitter caused by humans being humans (forgetful, lunch, meetings, etc), and there probably are not enough hours in the day for 1000 people to be deploying the same monolith. To increase residency time you can deploy separate units (You can have multiple deploym…

And I'm not even a little surprised:

https://engineering.shopify.com/blogs/engineering/introducin....

Folks, when you hire enough devs, they feel empowered to rewrite the world. I have lived all sides of this phenomenon and rarely is it pretty.

Scaling is a concern that goes in both directions. Shopify has 1000 developers today. How screwed would they be if they suddenly had to drop to 600? Or even if there's a hiring freeze? What happens when the people who wrote these tools go work somewhere else?

When I do tool smithing work these days, it's always with an effort to provide the thinnest of shims around open source or commercial tools with healthy user communities, so that at the end of the day they have a larger pool of resources than what is in house. People move on. Money dries up. Mandates change.

"Being important" in a company is about how much you support new work, not how locked in people are to your old work. If you can't give your old work away then you're shackling yourself, both to your current responsibilities and to the company. I can't believe that I'm the only one who has ever stayed at a company out of guilt for how screwed they'd be if I left. But that quickly turns into resentment which is worse.

If you are important for new work, then you always get new challenges. You stay sharp and your resume looks good. If the company stops doing new work altogether, do you really want to stay there anyway? Plus you could always go back to one of your old projects.

Re: Successfully Merging the Work of 1000 Developers

#62

Post by draw_down »

chrisseaton wrote: post #51
omeze wrote: post #43

Earlier quoted context omitted.

Bors when introduced at my workplace was summarily disabled after 2 separate incidents where it was improperly applying commit messages (which we patched internally) and improperly applying patches (at which point we disabled it and explored other solutions). Combined with the merge latency overhead, I would be extremely hesitant to advocate running it at scale.

> Bors when introduced at my workplace was summarily disabled after 2 separate incidents where it was improperly applying commit messages Seems a bit extreme to discard something after finding two bugs. Could you have fixed them instead?

Nope, that happening once is bad enough. Those aren't bugs like "oh shoot, my icon is the wrong color". It's really, really important that source control is not lying to you, or things will get very difficult very quickly.

Re: Successfully Merging the Work of 1000 Developers

#63

Post by BiteCode_dev »

chrisseaton wrote: post #51
omeze wrote: post #43

Earlier quoted context omitted.

Bors when introduced at my workplace was summarily disabled after 2 separate incidents where it was improperly applying commit messages (which we patched internally) and improperly applying patches (at which point we disabled it and explored other solutions). Combined with the merge latency overhead, I would be extremely hesitant to advocate running it at scale.

> Bors when introduced at my workplace was summarily disabled after 2 separate incidents where it was improperly applying commit messages Seems a bit extreme to discard something after finding two bugs. Could you have fixed them instead?

The problem is not the bugs, it's that it was probably recommended to them as a reliable solution, loosing them hours of work. This can't build trust, especially for such an important part of infrastructure: you'll always wonder when it's going to hit you next.

Re: Successfully Merging the Work of 1000 Developers

#64

Post by omeze »

chrisseaton wrote: post #51
omeze wrote: post #43

Earlier quoted context omitted.

Bors when introduced at my workplace was summarily disabled after 2 separate incidents where it was improperly applying commit messages (which we patched internally) and improperly applying patches (at which point we disabled it and explored other solutions). Combined with the merge latency overhead, I would be extremely hesitant to advocate running it at scale.

> Bors when introduced at my workplace was summarily disabled after 2 separate incidents where it was improperly applying commit messages Seems a bit extreme to discard something after finding two bugs. Could you have fixed them instead?

We did fix one bug, but after the 2nd incident decided that using a tool that silently corrupts your repository is not worth the marginal benefit it provides against alternative more naive solutions. Most companies run post-merge checks so in the event of a bad merge, the offending commit can be isolated and reverted, or have a cron running at a regular cadence to isolate the issue to a set of commits. Having your repo reflect developer’s patches is really table stakes for a tool like this anyway.

Re: Successfully Merging the Work of 1000 Developers

#65

Post by peterwwillis »

dkoston wrote: post #2

It would be useful in this article to hear about what content is acceptable in a merge request. For example: can these all go straight to queue because they use feature flags? Are commits a "single piece of work", etc. Not to sound like a downer but this is really an article about fixing a broken process because not running CI on branches before merging to master goes against best practices. Would have loved to actua…

> Trying to build CI on every branch before merging to master just won't work with the scale they are dealing with. At 1000 developers, the rate of PRs coming in makes it impossible to determine what current master will be when the PR is ready to merge (i.e. when the branch has a green build). It's also wasteful to build each branch against current master because what is "current" will not be when the branch is ready to merge.

I'm starting to think most CI problems are just people not looking at the problem the right way. Here is the problem re-worded:

- When a PR has a green light and someone hits 'merge', it locks anything else from being to merge to master, and you merge your PR. When it finishes merging and deploying, now all the other PRs waiting have to rebuild themselves to see if they will merge with this new state of master. So 100s of PRs are rebuilding every time you merge one PR, and there's constant CI churn.

Here is why that problem exists:

- The system was designed for 1000 developers to all be writing to the same code base.

Here is how you solve that:

- Don't let 1000 developers all write to the same code base. Break the code down into discrete components that different small teams manage. The only bottleneck for that code base is that small team.

This small team is often called the two-pizza team, and their discrete components are often called microservices.

Re: Successfully Merging the Work of 1000 Developers

#66

Post by demosito666 »

DilumAluthge wrote: post #28

This is exactly the kind of workflow that Bors ( https://github.com/bors-ng/bors-ng ) automates. In addition to Bors, there are a number of apps and services that automate this kind of workflow. Here is an incomplete list: https://forum.bors.tech/t/other-apps-that-implement-the-nrsr... Edited to add: Graydon Hoare (creator of Rust) called this the Not Rocket Science Rule Of Software Engineering (NRSROSE): "automatica…

I've read the description, but I fail to understand how this is different from just merging current master into the PR and running integration on that before merging in back into master? This can be done with 10 lines of groovy in Jenkins.

Also the exact workflow described with staging branch and batch merge is probably another 20-30 lines.

Re: Successfully Merging the Work of 1000 Developers

#67

Post by ljm »

demosito666 wrote: post #66
DilumAluthge wrote: post #28

This is exactly the kind of workflow that Bors ( https://github.com/bors-ng/bors-ng ) automates. In addition to Bors, there are a number of apps and services that automate this kind of workflow. Here is an incomplete list: https://forum.bors.tech/t/other-apps-that-implement-the-nrsr... Edited to add: Graydon Hoare (creator of Rust) called this the Not Rocket Science Rule Of Software Engineering (NRSROSE): "automatica…

I've read the description, but I fail to understand how this is different from just merging current master into the PR and running integration on that before merging in back into master? This can be done with 10 lines of groovy in Jenkins. Also the exact workflow described with staging branch and batch merge is probably another 20-30 lines.

This is what Travis CI did/does and I have not seen another CI platform offer the same.

Travis runs your test suite on the merge commit, not the head of the branch.

Re: Successfully Merging the Work of 1000 Developers

#68

Post by yoden »

jacktli wrote: post #35
dantiberian wrote: post #20

This sounds quite similar to https://bors.tech . If the authors are here, did you see this, and can you compare and contrast it with what you built? https://graydon2.dreamwidth.org/1597.html also has a good overview of the problem and the original bors.

Yes we have seen this before! The main difference is that throughput is extremely important for us, which we would not get worth Bors. Also, compatibility of multiple simultaneously merging PRs is the case that we are optimizing for, vs. compatibility with current master.

If you don't mind me asking, How long does a CI run take for you? How do you manage running CI with so many merges?

Our CI takes ~8 hours of machine/VM time, which is about 35 minutes of wall time with our current testing cluster (including non-distributed parts like building). We skip certain long tests during the day, so that brings wall time down to ~13 minutes. But we also test 2-3 branches with decent churn. So even if we're only doing post-merge CI based on the current state of master, we're still getting 5+ commits fairly often.

I want to get to a world where CI is run before and after each merge with master, on every commit (or push/pull), but it seems like it would take so much more resources and infrastructure than we currently have.

Re: Successfully Merging the Work of 1000 Developers

#69

Post by byroot »

dkoston wrote: post #59
byroot wrote: post #56

Earlier quoted context omitted.

No, even with v1, the merge weren't manual. A bot would merge for you, but directly into master. Now the bot merges into a temporary that is fast-forwarded as the new master if CI validates it.

Interesting. Would you say this is more of a decision based around the constraints of using GitHub or more of the ideal process for Shopify’s needs? I’m curious because the article doesn’t mention the core reasons that you chose to write your own CD tool versus the other options that exist. The workflow you describe seems readily available in most tools. Perhaps the throughput was causing other options to break?

The ideal process for Shopify’s needs based on the constraints we have to work with (CI speed, deploy speed, rate of changes, etc).

Re: Successfully Merging the Work of 1000 Developers

#70

Post by wvanbergen »

dkoston wrote: post #58
wvanbergen wrote: post #53

Earlier quoted context omitted.

A `develop` branch has several disadvantages. You will want to make your `develop` branch the default branch in git and on GitHub, to make sure pull requests automatically are targeted properly (not doing this would be a major UX pain). However, that means that when you `git clone` a repository you are not guaranteed to get a working version. The `develop` branch can still be broken, which is a problem that needs to…

Thanks, I was hoping for more of this in the blog post. Since tools are just an expression of process/policy, it’s more interesting to here about the process and why than it is about building “yet another CD tool”. Appreciate the thoughtful and thorough response. The major pain point I agree with on develop is changing the defaults to merge to that rather than master. It’s a shame this is not easier to do in git/gith…

> I guess I’m not fully understanding how a queue prevents this. Since you don’t have a full picture of the state of master until something is merged from the queue, how do the CI checks in the queue prevent things that branch-based CI checks wouldn’t prevent in a “develop” branch? With branches and develop, pull requests remain open until they can be assured they merge properly with develop as well.

The trick of the merge queue is that it splits the "merging a branch / pull request" in two steps:

1. Create a merge commit with master and your PR branch as ancestors.

2. Update the `master` ref to point to the merge commit.

Normally when you press the "Merge Pull Request" button, it will do those two things in one go. By splitting it up in two distinct steps, we can run CI between step 1 and 2, and only fast-forward master if CI is green.

This means that master only ever gets forwarded to green commits. And because the sha doesn't change during a fast-forward, all the CI statuses are retained. Only when we fast-forward will GitHub consider to pull request merged, so we don't have to "undo" pull request merges when they fail to integrate. If the merge commit fails to build successfully, we leave a comment on the PR that merging failed, and the PR is still open.

When we have multiple PRs in the queue, we can create merge commit on top of merge commit, and run CI on those merge commits. When once of these CI runs comes back, we can fast forward master to it, and potentially merge multiple pull requests at once with this approach.

Post reply on HN