Is it normal to have 3 to 5 devs working on the area of code so that one merged PR causes conflicts in the other PRs still in review?
Posted by RedbloodJarvey@reddit | ExperiencedDevs | View on Reddit | 52 comments
I'm relatively new to this company. For code quality and most processes they are well above other places I've worked. But this one reoccurring situation is taxing my feeling of productivity and a feeling of ownership of the project.
This is a very large team, working on a profitable project with tight external deadlines. As a new feature is started to be developed it will be broken up into several manageable tickets, which are assigned as developers finish up their previous work.
What this means is that a loose coalition of developers will be working on a feature, and often don't know who else is touching the same area of code. In the worst case scenario each time someone gets their PR through the approval process and merged in, everyone else has to refactor their code, which often means reworking unit tests, fixing linting issues, addressing code coverage gaps, etc. This adds hours, or days, and one time a full extra week to getting my PR in.
In a retrospective I brought this concern up. I was told to make smaller PRs. In my opinion that's not really practical. If a PR doesn't cover the ticket it will often get rejected during peer review.
Is this just the normal friction of working on a large project with tight deadlines?
smutje187@reddit
"As a new feature is started to be developed it will be broken up into several manageable tickets, which are assigned as developers finish up their previous work." - huge red flag as it sounds like some less technically capable person tries (obviously unsuccessfully) to separate code into bits that then lead to constant conflicts.
The solution is an easy split by vertical as people working on different verticals don’t have any conflicts other than maybe the occasional cross functional concern.
The hard part is to overcome the behavior ingrained into teams to blindly accept work handed down from above and pushing back/proposing alternative ways to organize work.
bobsbitchtitz@reddit
This is extremely normal. Good communication and pair programming here are must to execute effectively.
Specific_Ocelot_4132@reddit
IME it happens maybe once a month or so and the conflicts are usually minor and easy to resolve. If it’s worse than that, it can probably be avoided by coordinating better.
nasanu@reddit
This sounds like FE. And most comments sound like they come from BE devs. You basically can't work as a team on most FE projects, everything is tied to everything else, if you are doing it right anyway. CSS is global, components are global, utilities are global etc.
Like in BE you can make your own API route or some specific function which is just that function, its either there or its not and has zero impact on anything. On the FE right now I have tickets covering things to do with dates, near all screens have a date/calendar on them. But I also have another dev remaking the date component. Or I'll be required to add a field of data to something, but the others dont have that ticket and make types and state without that, so their changes will conflict with mine. Or just yesterday I needed to change validation for URLs, adding some params to pass to the validation function, making all pages that use that component have conflicts. No matter what there will be conflicts with too many FE devs (more than 1 is too many).
Also the small PR advice doesn't work. I can do.. well up 20 or so tickets a day. And if I dont put them into big PRs them ill pump out many PRs, but each one will need work in the others so my latest PR is always going to be a duplicate of the previous, it's going to be all PRs today till that point in time. It's really pointless as when finally say a few days later someone bothers to review a PR they start with the latest which makes all under it irrelevant.
Any-Ring6621@reddit
Anybody who pushes back against smaller PRs with “that’s not really practical” gets a red flag from me.
Making PRs smaller is almost always possible. But it sounds like your org may have a 1 PR per ticket rule (or you invented that yourself). If it’s the former, just create more tickets so you can make more PRs, it’s not like they cost anything, they’re just items in a board. It is extremely irritating, but, stupid orgs gonna do agile policy how they will.
If it’s the latter however, adjust yourself a little.
Tango1777@reddit
No, it's not normal. It occasionally happens, but overall it shouldn't be nowhere near normal.
matthedev@reddit
If I'm involved in planning, I try to make sure the work is distributed so that people aren't stepping on each other's toes too much if at all possible. If not, breaking down the code into smaller classes and functions and implementing the common parts first should help reduce the amount of merge conflicts.
PaleCommander@reddit
Coordinating with other devs on the same project or feature is exactly what stand-up is for, but if you're thinking ahead it can happen in sprint planning or the project kick-off meeting.
When this has happened to me, we've identified in in stand-up and coordinated the merge order and whether anyone should be basing their work on someone else's feature branches.
MyUsrNameWasTaken@reddit
This. This is exactly what stand-up is for (or better yet, sprint planning). If a dev says they are working on a file that you need to work on, alarm bells should be going off and a discussion on timing & merging should be had.
przemo_li@reddit
Serializing work is not the answer.
"Yeah, let's do it in twice the time, but I promise there will be less bugs!"
Planning is not the answer.
"Let's plan that refactor. Let me see, next decade, February 30th is ok for you?"
What is needed is evaluation of benefits. If those are good, optimizing for even more code churn is the answer!
5 people is too much right now?
Can we change our tools and processes so that 10 people in the same portion of the codebase is doable?
kondorb@reddit
That's typically achieved by not working on the same codebase, strictly speaking. I mean separating your code into separate "services". Putting it into quotes because I don't mean specifically network-separated microservices, it can be achieved by different architectures.
HolyPommeDeTerre@reddit
That's the way, if your conception phase is good enough, you can order the tasks by dependencies and reduce the overlaps or organise the intentional conflicts (feature branch, chaining branches...)
drumDev29@reddit
Yeah sounds like a branching strategy issue
Comprehensive-Pea812@reddit
can be an issue also if all developers aggresively refactoring all the codes.
separating part of works or keeping changes small is ideal, usually team lead would be able to helps in case members still new, but older members should be able to forecast potential conflict
PaleCommander@reddit
Yeah, declaring your in-flight big refactors or other large changes should be shared with the team as part of your status updates.
PaleCommander@reddit
Branching strategy will help, but step 1 is to identify the situation before everyone is in the middle of their work and get them talking to each other, which seems like the big missing piece in OP's case.
flowering_sun_star@reddit
For large code bases and organisations it's the sort of thing that can just happen by chance. Two teams are working on different projects that happen to touch the same area end up colliding. But if these are all developers working on the same project, and it's a regular occurrence, then this sounds like a flaw in how the work has been planned.
Of course the larger the project, the trickier it is to keep track of everything. But when I lay out the high level plan, I'll tend to take note of what areas might conflict or have dependencies. Maybe two things that touch the same area get folded into one ticket. Maybe one bit of work is marked as a dependency for another. Maybe create an early ticket to put in place a framework that subsequent tickets can flesh out without conflicts. When the team has a planning meeting to go over tickets, we'll pay attention to whether we reckon there will be conflicts.
How many developers I reckon can work at once without treading on each others toes is something I'll communicate to my manager, and that will contribute to the high level resource allocation.
And of course these initial plans won't be quite correct, and there will be some conflicts. That's where the standups come in - if you're aware of when others are working in the area, you can flag up that there may be an issue and work with that person to agree an approach.
Euphoric-Stock9065@reddit
There are "vertical" slices of work that can be worked on independently. These should be the bulk of the PRs.
Occasionally there need to be cross-cutting or "horizontal" slices of work that affect everyone - migrations, dependency updates, backup drills, refactoring, etc. For these, I've found nothing better than good ole human communication - figure out what, when, and who will do the work, call a feature freeze, schedule it, and do it - regularly. These tasks are inevitable but batching them makes sure the cross-cutting stuff gets addressed *but not in the main PRs*.
vigorthroughrigor@reddit
It happens, but if it consistently happens, then there's a problem.
przemo_li@reddit
Tradeoff.
Tradeoffs chosen but the team (or a company) may no longer fit the workflow the team wants to practice.
przemo_li@reddit
Consider trunk based development.
It really cuts on isolation of code. You literally see refactors being done on your code after each push/pull cycle. It's a huge benefit.
If you are still unconvinced, then stop and look at the benefits of the current system:
So maybe it's time to embrace the git strategy that lowers costs of those behaviors?
faultydesign@reddit
Constantly.
yoggolian@reddit
I think someone’s manager read the Mystical Man Month not the Mythical Man Month.
NoleMercy05@reddit
Yea
Huge-Leek844@reddit
Everyone is talking about breaking big PRs. But there is also the fact that PRs are open for a longer time than it should. Someone is not doing its reviews.
yxhuvud@reddit
Can you refactor that place to allow changes in parallel?
gHx4@reddit
Yes and no. It's normal to work on similar areas of code to complete deliverables for a feature set. Merge conflicts are also a normal part of development.
But, in order to make this work smoother, you should adopt practices that prevent and mitigate conflicts. Smaller, self-contained PRs is a great start. A rebase-oriented workflow also helps a lot, since you can 'defer' your commits for after the commits in a PR so that there's no interleaved commits to untangle. Communicating with your coworkers helps reduce the risk of you both doing the same work and having to reconcile the differences between two completely different MyFeatureFactory objects.
So your point in the retrospective was valid, but I do think that resolving conflicts is pretty normal workflow. If you have strategies for rebasing (correctly), PR conflicts shouldn't be taking more than 30-45 minutes.
Cautious_Implement17@reddit
hard to say exactly what the problem is here. could be one or all of:
yourself2k8@reddit
I'd throw in the task breakdown also. A mentality of "make smaller PRs" is great, but if the tasks are large its not always practical to make many small PRs to accomplish "a single task"
budding_gardener_1@reddit
I frequently deliver one ticket(if it's a large one) in multiple pull requests. If atomicity of a concern (i.e: this feature has to all to live at once) then an integration branch can be used so each PR is reviewed (ideally by different people to get lots of eyes on the code) into the integration branch and then the integration branch is finally PR'd (maybe with a quick "yeah that all makes sense to me" review) from someone (or multiple someones if you want) before it hits prod
danielt1263@reddit
This is absolutely the best answer so far. I will also add; if each developer is working on a different feature and they are all constantly stepping on each other's code, then you have very poor separation of concerns. (Which is something I've experienced in some large companies.)
One way to fix this would be to have each file owned by two people (two because you don't want to suffer from "hit by a bus" syndrome) and no two people should own the same set of files (stagger knowledge for good cross pollination). This doesn't mean you can't change a file you don't own, but it does mean you must get permission from owner before doing so. The owner of the file will know who else is working in it and can make sure changes are coordinated.
zemdega@reddit
Pretty common to have to rebase/merge from upstream due to PRs landing, especially in very active open source repositories. I wouldn’t worry about it too much, just communicate with each other.
kbielefe@reddit
I would say merge conflicts are normal, but being surprised by merge conflicts is abnormal and problematic. The bigger problem is not the conflicts git detects, but the conflicts git can't detect. The only real way to mitigate those is frequent communication so you know how your changes interact.
budding_gardener_1@reddit
It's not normal no but it can happen. The way to go is to flag it up early and don't be a dick about it
Eric848448@reddit
We certainly try to avoid that at my company.
I worked at a place briefly that would have an entire team swarm around one area and constantly stomp on each other’s changes. That’s one of many reasons that place sucked.
Main-Eagle-26@reddit
Yes. And a good well made dev plan will be one that avoids conflicts as best as possible.
This is what slack and standup is for. A big part of your job is communicating and collaborating to avoid these kinds of conflicts.
ThatSituation9908@reddit
Just to add a new perspective, the open source (open contribution) works despite lacking cross-coordination of devs. For very large OSS projects, you can see how small most PRs are. You can also see large PRs require a ton of coordination from the maintainers.
Push for small PRs, push for multiple PRs in one ticket.
behusbwj@reddit
The real question is why are you all working on the same code instead of reviewing each other’s codes fast enough that conflicts don’t happen that often. The root cause of merge conflicts on my teams is 9/10 of the time backed up code reviews
aseradyn@reddit
We run into this a lot.
A few things have helped:
Writing more modular code. If we break code up into smaller functional units, we avoid some of the messier merges, because not everything is interwoven in the same file/method.
Dividing or recombining cards specifically with an eye to reducing messy merges. These three bits will need to work together, so they need to either be built in series or the interlocking parts need to be done together.
Planning a new feature as a group, and prioritizing stories so that key hub pieces are done early and fast so that additional pieces can be bolted on in parallel.
Convincing the PM to have more than one project going at a time, to reduce the number of devs working in the same files. The metaphor I like is a group of people working on a jigsaw puzzle. If the puzzle is on a tiny table, there's a limit to how many people can actually see and reach the pieces to help complete it. If 3 people fit around the table, the other 5 can twiddle their thumbs, push the first three out of the way to "help", or go find another puzzle to work on.
tjsr@reddit
Yes, it's normal. No, it should not be, and indicates problems with the way teams are planning and communicating.
lorryslorrys@reddit
PR size is the problem, they should be much smaller. A day's work is a good upper limit to shoot for. Being able to add a week's work to a PR is madness. The benefit of continuous integration are well documented and evidenced (DORA).
What do you mean you were told to make them smaller, but also your small PRs were rejected? Aren't these the same people?
Are you making small and correct changes? Are you using abstract and features flags to hide code that can't run until the feature is done?
It's also probably the case that people, at the very least those working on the same feature, should be communicating better. Probably even pairing (although that would require a culture that prioritizes effectiveness over typing throughput, which doesn't tend to go with "very large dev team")
superdietpepsi@reddit
Sounds like an architecture issue at the code level or the team is breaking the tickets down inefficiently
GandolfMagicFruits@reddit
☝️
al2o3cr@reddit
IMO this is the most immediately actionable thing you could try to fix: if multiple people are working on the same feature, they need to be communicating with each other.
That communication can sometimes spin off additional tickets that are prerequisites or preparatory refactors, which can reduce merge conflicts by making dependencies between changes more explicit.
Longer-term you'll want to consider your process for allocating work; sometimes leadership will convince themselves that they can boil an egg in 18 seconds by putting 10 in the pot at once.
Longest-term, if you're regularly getting big conflicts in the same files definitely consider if there's a better architectural approach to keep things that change independently separated.
low_slearner@reddit
Sounds like you need smaller, more frequent PRs all round. A PR should achieve something, but it doesn’t have to implement a whole ticket. If you can’t sell people on that, make much smaller tickets.
Other things to consider: - Is your codebase a big ball of mud? If you’re all working on separate things but your PRs keep causing conflicts on other branches, then you need to modularise better. - Pair programming. Fewer branche, no need for separate code reviews, and (sometimes) faster implementation leads to fewer conflicts, as well as all the other benefits.
marmot1101@reddit
This is a bit of a smell to me. If you have to do a non-trivial refactor based on someone else's merge then the communication about how things are going to be implemented wasn't good enough. If you're dealing with different pieces of the app then you can get away with a little bit of silo'ing. If you're all going to be playing in the same sandbox you probably need to be talking with each other really often. If unit tests are granular tests of one function, and unit tests have to change, that indicates that either method contracts are changing out from under you(again, that's a communication and/or coding fault) or you're all in the same method. If it's the later that method is really big and broad or the tickets are too granular. If you have to change full stack functional tests that makes sense, but even still that shouldn't lead to long refactors, more like resolving conflicts and maybe slight changes to the test.
It's probably worth pressing the issue in another retro. I would document how much time was spent resolving merge conflicts and re-work based on merge order. "Smaller pr's" is kind of a handwavy answer to a problem that's effecting productivity. If it's costing you a week(and I've been in that situation) that's a problem expensive enough to warrant some deeper thought.
No-Economics-8239@reddit
Normal? Probably not. But I've had sprints where multiple tasks popped up that all targeted different parts of the same codebase. It's not completely unheard of for a given piece of code to suddenly come under business scrutiny. And if there is a priority to push it along, they may not want to wait for a single coder to complete all the tasks.
So we coordinated together over who was doing what. And then there was an informal race to be the first to commit, so the others had to deal with the merge conflicts. But there was an eye to watch the other branches to see what was coming and avoid stepping on virtual toes.
robk00@reddit
Split the ticket into multiple smaller tickets so you can merge your code frequently.
How often do you merge your PRs when working on a feature? I usually aim to create and merge a new PR every single day. And this usually works as every PR should be reviewed under 24 hours.
Osr0@reddit
I've never worked for a big software company, but in over 20 years of being a developer I've never been in an environment anything like this.
This sounds absurdly inefficient. Its one thing to have a client/server or endpoint/consumer type relationship where you'll need to periodically account for unplanned changes to parameters, DTOs, etc. but holy shit man this sounds balls to the wall crazy.
Perhaps it is just due to the environments I have experienced, but I would think a big part of the purpose of all the management and overhead you're dealing with would be to fucking avoid scenarios like what you're describing.
zica-do-reddit@reddit
Talk about it in the retrospective, sounds like you have "God classes" and you may need to refactor a bit.
Dangerous-Quality-79@reddit
Yes, this is pretty normal. Devs need to communicate constantly and share often. If you find a bug that other people will be affected, make a commit and tell others to cherry pick it. Any team event is about communication.
duddnddkslsep@reddit
Yes, it's expected. Smaller PRs make it manageable. Having a huge PR is also not justifiable in terms of getting your code reviewed by other devs.