1. 12
  1.  

  2. 7

    The author overlooks that, often times, the cognitive load of engaging with the owner of a project is greater than “scratching the itch”. My approach in that situation is to scratch whatever “itch” I have to unblock my task at hand. Then, when I have time and if I find my patch worth submitting, I follow the guidelines of the project and submit a PR. I attach no pride to that submission and expect it to be rejected for whatever reason. When that happens, I am usually happy to rework the patch.

    1. 5

      In addition to this, I feel the “Why are you making this change? What problem are you trying to solve?”-question should have been the first comment on the PR, instead of arguing that “it’s a bad change”. People usually don’t make these kind of changes for the fun of it.

      1. 3

        I would say that to me, this goes backwards. If someone offers a change, I expect them to explain why they’re going it.

        Communicating the intent before is always better than having to ask why after it’s done.

        The author even gives a the very particular case of a distributed team, where a team might have lost hours of implementation even before spending 10minutes in discussion.

        1. 1

          Well, ideally, yes. But people get caught up in their reasoning at times and assume it’s all just as obvious to other people as it is to themselves. Sometimes it is; often it’s not. It’s a simple human mistake that I think we’ve all made at least once or twice.

          Either way, don’t start argueing over the how if the why isn’t clear, which seems to have happened in the OP’s case.

          1. 1

            Agreed. I feel this often happens with strong ownership models.

      2. 1

        Good point. I’m the OP and perhaps I could have made it more clear that I’ve seen this issue happen more frequently with distributed teams in different timezones, especially when trying to make changes to less mature codebases owned by another, remote team.

        As I’ve observed these instances from a step back, I found it wild how narrow the person proposing the change was thinking, and how much time they poured into making a change that never made sense from the platform’s point of view. And these are smart engineers, who were under pressure to solve their pressing problem, on the spot.

        I’ll give you an example: a customer of a platform was frustrated how localization changes took around a day for the system to pick up. And this was blocking their team. They noticed that whenever the service was restarted, localization changes got picked up immediately. So they put together a pull request, implementing a way to restart service instances on-demand, when localization changes were done. They didn’t think further on what restarts would mean in a distributed world, how this could impact jobs, workflows and the many customers of this platform. And they added no commentary on the pull request.

        Had they talked with the platform team, outlining the problem, they would have learned that this was a known issue and the way it was being solved is… well, the platform emptying localization cache. It was just not a priority until then, and the platform engineers were unaware of the issue this caused.

      3. 2

        We follow an internal open source model with strong code ownership, where any engineer can put up pull requests for codebases. The owning team of that system sets up blocking code reviews.

        Worked at a place where this was practiced. A few core teams (10-20 people) blocked the rest of the company. The fan-in was too great. Blame the org chart or the process. Worked at other places where CRs/PRs are a courtesy, where automated testing keeps most of the bad stuff out and where you fix what you break. I felt that people in that environment were more open and willing to discuss things beforehand too. Less hierarchy, less gatekeeping. YMMV

        1. 2

          Thanks for sharing your experience. I guess it all depends on the environment. At Uber, there are hundreds of services, each service owned by a team, all of them doing blocking code reviews for their own services. It evolved like this, after the alternative - anyone pushing changes that made sense to them - quickly turning into chaos, building up large tech/architecture debt. My team is just untangling such a case, where a central library had no blocking reviewers, and over the course of a year, it’s turned from a reusable library to a hot piece of mess, with dependency and maintenance hell, after engineers were making changes that scratched their itch.

          Having an owner for a service, who makes sure it is changed in a strategic, over a tactical way, is a pretty good thing.

          There are cases when a team/service becomes a bottleneck. In this case, I’ve seen two things happen. First, put an SLA for reviews in place: usually 24 hours. If that SLA is not met, teams are free to push changes. Second, start a negotiation on how to remove the blockage. This is usually resolved by either carving off a part of a service/codebase to a separate service or code ownership, that a smaller team owns and decides to implement blocking code reviews (or not). The other approach is to formalize “core” code review requirements, then onboard members of other teams to be these core reviewers, helping keep the quality bar high.

          If a small group is blocking a larger group, that is a bad thing and something needs to change. Sounds like therer was little flexibility at that company to do so?