4 ms·

I do not really agree with the conclusion. Google does large scale automatic refactorings. Those clearly benefit from atomic commits, because it is easy to roll

I do not really agree with the conclusion. Google does large scale automatic refactorings. Those clearly benefit from atomic commits, because it is easy to roll them back in that case. As other have mentioned in smaller projects you might want to be able to do some (internal) refactorings easily and being able to roll them back easily is a big advantage

skybrian 5y ago
I’m not sure what you consider the conclusion to be, but large-scale refactorings at Google do happen in the way described in the article, with the bulk of the migration done using many commits done in parallel. Being able to rollback is indeed important and reducing the sizes of commits makes it less painful. If one project has a problem, unrelated projects won’t see the churn. Or at least that’s how it was done when I was there. It’s been a while.
- kohlerm 5y ago
  I am saying large-scale refactoring can and should (in same cases) happen (and are doable). The author claims you should always do it in steps. I disagree with this opinion. If the refactoring is automatic, why would I do it in steps?
  - skybrian 5y ago
    The more projects you touch, the more approvals you need and the more tests you need to run. Some tests take a long time to run and some are flaky. And the longer to takes to do all this, the more likely there is to be a merge conflict due to other people’s commits. If you divide it up, most commits will land quickly and without issue, and then you can deal with the rest.
  - bluGill 5y ago
    Is your automatic refactoring completely 100% bug free in all edge conditions? I personally don't have that much confidence.
- joatmon-snoo 5y ago
  It still is done that way, but even individual LSC (large scale change) CLs touch easily >100 files and rely very strongly on the atomicity guarantee of the monorepo. Plus, when most people think LSCs, they think about the kind of stuff that the C++ or TS or <insert lang> team do, not someone refactoring some library used by a handful of teams, which themselves usually impact anywhere between ten to a few hundred files.
tylerhou 5y ago
As someone who has worked on large-scale refactorings at Google they usually do happen as the author describes: 1. Add the new interface (& deprecate the old one) 2. Migrate callers over (this can take a long time) 3. Remove the old interface. Even then, you risk breakages because in some cases the deprecation relies on creating a list of approved existing callers, and new callers might be checked in while you're generating that list. (In that case you would ask the new callers to fix-forward by adding themselves to the list.) This three step process has to happen because step 2 takes a long time for widely-used interfaces, and automatic refactorings cannot handle all cases. The only time you can consolidate all three into one commit is if the refactoring is so trivial that automatic tooling can handle every case (in which case, does the cost of code churn & code review really justify the change?) or the number of usages are small enough that a person can manually make all the changes the automatic fixer can't handle before much churn happens.
- vlovich123 5y ago
  Hmm... I didn't work on large scale refactors but (IIRC - it's been a few years) I definitely had to approve commits that were touching components I had ownership over & that was part of the same commit as many other changes batched. Given that I recall a wiki describing what your responsibility was as the downstream user of the API in these scenarios, it seemed like a common thing to me & I recall there were also automated tools to help do this. Whether or not that's the most common workflow is another story. Works great at the C++/Java API layer or trivial reorganizations, may not work as well when modifying runtime behavior since you have to do the 3 phase commit anyway.
  - tylerhou 5y ago
    I think it depends on how large the change is. If the change is small — e.g. we need to slightly modify the signature of some function, then the library maintainer would be responsible as they can easily reason about the change (or automatically generate the change) and there is not much benefit to asking the consumer to generate it. Approving commits is part of any (non-global approval) LSC process; originally I meant that there's a lot more friction in a consumer generating code changes than approving them. If you received an email saying that you had to make a bunch of small changes to your codebase, you would probably ignore it and keep working on your P1/P2; on the other hand if you receive a small, reasonable CL from a library team updating their function usages you would likely LGTM without much thought. On the other hand, if the change is: we need to completely overhaul the interface of this library/service, then you need more in-depth reasoning and the consumer team is responsible for making the change (similar to deprecating a service), since these large "deprecations" require a lot more knowledge of how consumers use the code. There is also a middle ground where the changes are somewhat easy to reason about but still require some human intervention; then the task is usually assigned to volunteers.

skybrian 5y ago

I’m not sure what you consider the conclusion to be, but large-scale refactorings at Google do happen in the way described in the article, with the bulk of the migration done using many commits done in parallel. Being able to rollback is indeed important and reducing the sizes of commits makes it less painful. If one project has a problem, unrelated projects won’t see the churn. Or at least that’s how it was done when I was there. It’s been a while.

kohlerm 5y ago
I am saying large-scale refactoring can and should (in same cases) happen (and are doable). The author claims you should always do it in steps. I disagree with this opinion. If the refactoring is automatic, why would I do it in steps?
- skybrian 5y ago
  The more projects you touch, the more approvals you need and the more tests you need to run. Some tests take a long time to run and some are flaky. And the longer to takes to do all this, the more likely there is to be a merge conflict due to other people’s commits. If you divide it up, most commits will land quickly and without issue, and then you can deal with the rest.
- bluGill 5y ago
  Is your automatic refactoring completely 100% bug free in all edge conditions? I personally don't have that much confidence.
joatmon-snoo 5y ago
It still is done that way, but even individual LSC (large scale change) CLs touch easily >100 files and rely very strongly on the atomicity guarantee of the monorepo. Plus, when most people think LSCs, they think about the kind of stuff that the C++ or TS or <insert lang> team do, not someone refactoring some library used by a handful of teams, which themselves usually impact anywhere between ten to a few hundred files.

tylerhou 5y ago

As someone who has worked on large-scale refactorings at Google they usually do happen as the author describes: 1. Add the new interface (& deprecate the old one) 2. Migrate callers over (this can take a long time) 3. Remove the old interface. Even then, you risk breakages because in some cases the deprecation relies on creating a list of approved existing callers, and new callers might be checked in while you're generating that list. (In that case you would ask the new callers to fix-forward by adding themselves to the list.) This three step process has to happen because step 2 takes a long time for widely-used interfaces, and automatic refactorings cannot handle all cases. The only time you can consolidate all three into one commit is if the refactoring is so trivial that automatic tooling can handle every case (in which case, does the cost of code churn & code review really justify the change?) or the number of usages are small enough that a person can manually make all the changes the automatic fixer can't handle before much churn happens.

vlovich123 5y ago
Hmm... I didn't work on large scale refactors but (IIRC - it's been a few years) I definitely had to approve commits that were touching components I had ownership over & that was part of the same commit as many other changes batched. Given that I recall a wiki describing what your responsibility was as the downstream user of the API in these scenarios, it seemed like a common thing to me & I recall there were also automated tools to help do this. Whether or not that's the most common workflow is another story. Works great at the C++/Java API layer or trivial reorganizations, may not work as well when modifying runtime behavior since you have to do the 3 phase commit anyway.
- tylerhou 5y ago
  I think it depends on how large the change is. If the change is small — e.g. we need to slightly modify the signature of some function, then the library maintainer would be responsible as they can easily reason about the change (or automatically generate the change) and there is not much benefit to asking the consumer to generate it. Approving commits is part of any (non-global approval) LSC process; originally I meant that there's a lot more friction in a consumer generating code changes than approving them. If you received an email saying that you had to make a bunch of small changes to your codebase, you would probably ignore it and keep working on your P1/P2; on the other hand if you receive a small, reasonable CL from a library team updating their function usages you would likely LGTM without much thought. On the other hand, if the change is: we need to completely overhaul the interface of this library/service, then you need more in-depth reasoning and the consumer team is responsible for making the change (similar to deprecating a service), since these large "deprecations" require a lot more knowledge of how consumers use the code. There is also a middle ground where the changes are somewhat easy to reason about but still require some human intervention; then the task is usually assigned to volunteers.