>In terms of speed, we find that developers have to wait for initial feedback on their change a median time of under an hour for small changes and about 5 hours for very large changes. The overall (all code sizes) median latency for the entire review process is under 4 hours
I worked at Google from 2014-2018. I'm extremely skeptical of these numbers.
Based on my experience working on three different teams and sending changelists to many other teams outside my own, I'd expect something more like 3-4 hours for small changes (<50 LOC) and 1-2 days for large changelists (500+ LOC).
Google has so many automated changelists that I'm wondering if that skewed their numbers. Like if someone just clicks through 10 auto-generated CLs and approves them all, did that count for a bunch of near-instant review cycles?
That said, I was a big fan of Google's code review process. I know people complained about the unnecessary level of process and the hassle of getting readability, but I found it all a great way to spread knowledge across teammates and improve programming skills.
I think pocket reviewers probably account for the generally short time. In my day-to-day I usually did not send changes to people who weren’t already primed to receive them and sitting right next to me, so I could turn around and say “I sent you the thing” and they’d review it straight away because we were working towards the same goal anyway, and in some cases had already pair-designed or pair-coded the thing so the review was already a formality.
That's definitely curious. If the median latency is under 4 hours, does that mean there are lots of people sitting around doing nothing, or working on nothing urgent, so that they can immediately turn around a code review?
By means of 12 interviews, a survey with 44 respondents, and the analysis of review logs for 9 million reviewed changes, we investigate motivations behind code review at Google, current practices, and developers’ satisfaction and challenges.
While quite a large number of changes, that seems a rather small sample size for interviews, and especially for survey response.
A general pattern of tech folks trying to understand a subject is that they will analyze billions of data points while doing everything in their power to avoid or minimize talking with people.
I worked at Google circa 2015, and found the code review process to be actively terrible.
I was writing JS, while my sole teammate was a firmware engineer. He was not willing or able to meaningfully review my code, so I had to try to scrounge random reviewers from other teams. They naturally de-prioritized it, so it took forever. And because they were not involved in my project, they would only have local stylistic feedback.
Months in, I finally figured out that other engineers were just self-reviewing: you can add yourself as a reviewer and just click merge. So I started doing that too.
That honestly sounds very atypical. I've been at Google 9 years and never experienced the scenario you describe. You should probably have reached outside of your team for assistance or flagged a manager.
Yes, there have been times when code review wasn't super useful, but on the whole I never experienced the kind of dysfunction you're talking about and merging reviews without external review is a pretty big no-no.
Your comment seems to be exactly the same as this one [1] from the original HN story in 2018, except with less detail. Are you ridiculous_fish, or is this just a huge coincidence or something that happens all the time at Google?
> Months in, I finally figured out that other engineers were just self-reviewing: you can add yourself as a reviewer and just click merge. So I started doing that too.
Where? This certainly isn't possible in google3 (and wasn't possible in 2015 either).
That said, there are now some changes made to make, in general, the code review process for people without readability and without teammates who have readability more streamlined.
Sounds like a team health issue. Sounds like a hard time, my sympathies.
In my N years of experience, very few engineers are in an environment where only they have meaningful context on what they're working on. Generally any non-trivial project worth staffing is worth having two headcount work in the space, even if one is a TL with divided attention.
The review experience can really vary between teams, and depending on whether you have readability or not and whether you're changing code you own or not. This can make the difference between a couple of hours to a week to submit the same change. Readability in general is a process that while valuable, but can be really streamlined. I was working in a fast moving team using Python where everyone had readability, and I just didn't have time to delay most non trivial CLs, so even after submitting hundreds of CLs I did not have readability, having sent only a handful for review. Another negative effect I saw was people opting not to use a less popular language that was the right choice for a project, just because not many people in their office have readability in that language.
A much easier process would go something like this: Suppose my teammate has readability in language X, They can tag review comments as being readability related or not. Once someone has N CLs under their belt with readability related comments under some low percentage in the past M CLs, they are granted readability automatically, or have to submit just 1-2 CLs for a speedy readability process.
I'm realizing you are using "have readability" in a specialized way I may not be following. Can you say more what you mean by "whether you have readability" and "readability is a process...". Readability is a process (not an adjective)? That only some teams have? Can you say more?
Yeah python and c++ readability there is full on bs. No two readability reviewers agree on things, nitpicking was ridiculous and very superficial ime and most of those issues can be solved with some light linting/static analysis. Then you watch all those l6s+ shipping obviously terribly styled code bc noone dares to say shit... fwiw Go had much better process for this but i got mine when the readibility list was like a hundred folks so not sure about now
Modern was a poor choice of words for this title. It's three years old at this point and will only become less applicable over time. "A 2018 Investigation Into Code Review at Google" might have aged better.
[+] [-] mtlynch|5 years ago|reply
I worked at Google from 2014-2018. I'm extremely skeptical of these numbers.
Based on my experience working on three different teams and sending changelists to many other teams outside my own, I'd expect something more like 3-4 hours for small changes (<50 LOC) and 1-2 days for large changelists (500+ LOC).
Google has so many automated changelists that I'm wondering if that skewed their numbers. Like if someone just clicks through 10 auto-generated CLs and approves them all, did that count for a bunch of near-instant review cycles?
That said, I was a big fan of Google's code review process. I know people complained about the unnecessary level of process and the hassle of getting readability, but I found it all a great way to spread knowledge across teammates and improve programming skills.
[+] [-] jeffbee|5 years ago|reply
[+] [-] lupire|5 years ago|reply
Sure, you can split out 9 little cleanups and typo fixes while working on a larger change, but it's still days to get the main change approved.
[+] [-] throwawaysea|5 years ago|reply
[+] [-] dang|5 years ago|reply
Modern Code Review: A Case Study at Google [pdf] - https://news.ycombinator.com/item?id=18035548 - Sept 2018 (64 comments)
[+] [-] kbenson|5 years ago|reply
While quite a large number of changes, that seems a rather small sample size for interviews, and especially for survey response.
[+] [-] mediaman|5 years ago|reply
[+] [-] OneMoreGoogler|5 years ago|reply
I was writing JS, while my sole teammate was a firmware engineer. He was not willing or able to meaningfully review my code, so I had to try to scrounge random reviewers from other teams. They naturally de-prioritized it, so it took forever. And because they were not involved in my project, they would only have local stylistic feedback.
Months in, I finally figured out that other engineers were just self-reviewing: you can add yourself as a reviewer and just click merge. So I started doing that too.
[+] [-] cmrdporcupine|5 years ago|reply
Yes, there have been times when code review wasn't super useful, but on the whole I never experienced the kind of dysfunction you're talking about and merging reviews without external review is a pretty big no-no.
[+] [-] atombender|5 years ago|reply
[1] https://news.ycombinator.com/item?id=18037184
[+] [-] joshuamorton|5 years ago|reply
Where? This certainly isn't possible in google3 (and wasn't possible in 2015 either).
That said, there are now some changes made to make, in general, the code review process for people without readability and without teammates who have readability more streamlined.
[+] [-] B-Con|5 years ago|reply
In my N years of experience, very few engineers are in an environment where only they have meaningful context on what they're working on. Generally any non-trivial project worth staffing is worth having two headcount work in the space, even if one is a TL with divided attention.
[+] [-] dandigangi|5 years ago|reply
[+] [-] Goog_l_Dude_l|5 years ago|reply
A much easier process would go something like this: Suppose my teammate has readability in language X, They can tag review comments as being readability related or not. Once someone has N CLs under their belt with readability related comments under some low percentage in the past M CLs, they are granted readability automatically, or have to submit just 1-2 CLs for a speedy readability process.
[+] [-] jrochkind1|5 years ago|reply
[+] [-] dilyevsky|5 years ago|reply
[+] [-] cosmotic|5 years ago|reply
[+] [-] tantalor|5 years ago|reply