5 ms·

Xoogler here, circa 2014. Noogler orientation impressed upon us that every change was code reviewed, period! However my experience was that code reviews were he

Xoogler here, circa 2014. Noogler orientation impressed upon us that every change was code reviewed, period! However my experience was that code reviews were heavyweight and frustrating, often to the point of just being skipped: completely different from the article's observation of latency in hours and "fast-paced iterative development." 1. My initial team was a firmware engineer, and myself doing Android development. My peer felt unqualified to review Android code, so I spent a lot of energy begging code review time from tangentially related teams. Their feedback was not deep or useful for obvious reasons, and the days of latency posed a major problem because our project was tied to a hardware schedule. I learned later my teammate was simply stamping his own changes, which the tool permits, and so I started doing the same. Bypassing code review resolved the problem. 2. My next team was mostly in East Asia, while I was in Mountain View. If they had a question on a change, it would add two days latency due to time zone differences. And because I was physically detached from my peers, I believe they felt more comfortable deferring my changes. Again we were tied to a hardware schedule, so this latency was very painful. 3. The last scenario was overseeing builds, on (say) a Foxconn factory floor. For every build, the hardware coming off the line would behave unexpectedly, and we would have to very quickly update some code to make things work. Code reviews in this situation would have been absurd: any changes only had to last as long as the build itself, and well, there wasn't even network access. This is admittedly an exotic and specialized scenario (but recall orientation's "every change, period!") As a Googler code reviews were either skipped or were a major burden. I made some mistakes: I ought to have escalated quicker, increased my visibility, been less cowed by the orientation. Google also could improve in some areas: 1. Code review expectations need to be adjusted for small heterogenous teams. Such teams are more likely as Google ramps up its hardware efforts. 2. The engineering culture ought to recognize the burden imposed by time zones. Code reviews must be more forgiving when the communication cost is high. 3. Shift orientation towards teams instead of company-wide. CR for self-driving cars must be fundamentally different than CR for matching selfies with famous artwork, or CRs for updating factory floor test fixtures. Some of these may have changed since I left and if so I'd love to hear how.

cpeterso 8y ago
Mozilla requires code review for all code changes, too. Time zones are a big challenge because many employees work remotely and few feature teams are collocated in the same office. Engineers have to accept some latency, like you describe. To keep their pipeline full, an engineer typically must multitask multiple bugs in different stages of the review process. Reviewers try to prioritize requests from volunteer contributors so they don't get discouraged. These reviews often require extra feedback on basics of patch creation or coding style. Over time, some experienced volunteer contributors become reviewers, too. Instead of requesting review from a specific individual, some teams have experimented with assigning some review requests to a team alias. Anyone on the team who is available or has some free time can approve and land the patch. This works well for small patches that don't depend on specific expertise. This process also depends on everyone sharing the review burden.
nitwit005 8y ago
I have also seen the "mandatory code reviews" bit fall apart with teams in mixed time zones. One tiny back and forth with the Paris team can delay a change by two days, sometimes blocking other work. I have wanted to switch to something closer to "just commit, notify people, and let them revert or fix it as needed".
joatmon-snoo 8y ago
A lot of your perspective seems to be the result of a very strange work situation. If you regularly need people on the other side of the world to review complex changes, then your team shouldn't have an ocean between its halves. That being said: 1. Given that OWNERS approval must come from your own team, yes, your team dictates many of the code expectations. I'm not sure how this was ever not the case, unless you're arguing that teams should be exempt from readability, in which case my rebuttal is that although people gripe about it a lot, the vast majority agree on its utility after they go through the process. (Rereading your comment it sounds like you had difficulty getting Java approvals, and today there is a system where you can request a review from the pool of Java readability approvers at Google, and the turnaround tends to be pretty good, generally within a few hours.) 2. Absolutely not. If anything those need to be more careful to make sure nothing's lost in communication. 3. See 1. This is also driven by general test coverage and design processes, as well as the ability to define custom presubmit checks on a path-by-path basis.

cpeterso 8y ago

Mozilla requires code review for all code changes, too. Time zones are a big challenge because many employees work remotely and few feature teams are collocated in the same office. Engineers have to accept some latency, like you describe. To keep their pipeline full, an engineer typically must multitask multiple bugs in different stages of the review process. Reviewers try to prioritize requests from volunteer contributors so they don't get discouraged. These reviews often require extra feedback on basics of patch creation or coding style. Over time, some experienced volunteer contributors become reviewers, too. Instead of requesting review from a specific individual, some teams have experimented with assigning some review requests to a team alias. Anyone on the team who is available or has some free time can approve and land the patch. This works well for small patches that don't depend on specific expertise. This process also depends on everyone sharing the review burden.

nitwit005 8y ago

I have also seen the "mandatory code reviews" bit fall apart with teams in mixed time zones. One tiny back and forth with the Paris team can delay a change by two days, sometimes blocking other work. I have wanted to switch to something closer to "just commit, notify people, and let them revert or fix it as needed".

joatmon-snoo 8y ago

A lot of your perspective seems to be the result of a very strange work situation. If you regularly need people on the other side of the world to review complex changes, then your team shouldn't have an ocean between its halves. That being said: 1. Given that OWNERS approval must come from your own team, yes, your team dictates many of the code expectations. I'm not sure how this was ever not the case, unless you're arguing that teams should be exempt from readability, in which case my rebuttal is that although people gripe about it a lot, the vast majority agree on its utility after they go through the process. (Rereading your comment it sounds like you had difficulty getting Java approvals, and today there is a system where you can request a review from the pool of Java readability approvers at Google, and the turnaround tends to be pretty good, generally within a few hours.) 2. Absolutely not. If anything those need to be more careful to make sure nothing's lost in communication. 3. See 1. This is also driven by general test coverage and design processes, as well as the ability to define custom presubmit checks on a path-by-path basis.