[HN Gopher] Modern Code Review: A Case Study at Google (2018) [pdf]
___________________________________________________________________
Modern Code Review: A Case Study at Google (2018) [pdf]
Author : ra7
Score : 59 points
Date : 2021-03-31 17:00 UTC (6 hours ago)
(HTM) web link (sback.it)
(TXT) w3m dump (sback.it)
| mtlynch wrote:
| > _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.
| jeffbee wrote:
| 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.
| lupire wrote:
| Even if the median or 90%ile review is quite quick, the slow
| ones are where all the pain is.
|
| 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.
| kbenson wrote:
| _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.
| cosmotic wrote:
| 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.
| tantalor wrote:
| What has changed in 3 years?
| cosmotic wrote:
| Within Google? I suspect enough to make this paper obsolete.
|
| The paper itself says: "Employing lightweight, tool-based
| code review of code changes (aka modern code review)"; I
| think that substitution was a disservice. Practically all
| case studies are on modern processes so it's not really worth
| specifying.
| wffurr wrote:
| After a quick scan of the article, I can tell you that not
| much has changed in the interim.
| cosmotic wrote:
| Will that hold true for the rest of time? If not, a more
| descriptive word would have been better than 'modern'.
| OneMoreGoogler wrote:
| 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.
| dandigangi wrote:
| Self merging your code at a company like Google is wild.
| throwaway53453 wrote:
| That does not sound like they were working in the main trunk.
| I do not believe you can run any production code that was
| self signed like this.
| jeffbee wrote:
| You can't. Borg binary authentication is integrated with
| build and review. A build that does not descend entirely
| from reviewed, approved, and committed code running as a
| production user with access to userdata will raise alerts.
| Individuals are able to run non-committed code on Borg
| under their own accounts, but not under production role
| accounts.
|
| You can break glass in emergencies by committing code TBR,
| or "to be reviewed", however this automatically escalates
| to owners of the code in question plus your manager and
| director, and all TBRs have to be resolved by actual review
| within a short time. An author cannot submit to-be-approved
| code; they have to be owners of the code in question
| (personally or transitively included in the OWNERS file) to
| TBR.
|
| You can read about this system here:
| https://cloud.google.com/security/binary-authorization-
| for-b...
| atombender wrote:
| 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?
|
| [1] https://news.ycombinator.com/item?id=18037184
| dandigangi wrote:
| Rekt
| OneMoreGoogler wrote:
| Ha, busted.
| joshuamorton wrote:
| > 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.
| OneMoreGoogler wrote:
| It was in the Android repo, which used Gerrit.
|
| I never dealt with readability, because Android did not
| require it. In fact that was part of their internal
| recruiting pitch: fed up with readability? Come work on
| Android! Heh.
| gct wrote:
| Ah yes, that's been rectified now =D
| packetslave wrote:
| well, Android was always a bit of a special snowflake
| compared to the rest of the company. Separate codebase,
| separate build system, separate SCCM, etc. You can't really
| compare an Android team to a google3 team.
|
| Culturally, it was worse when Andy was in charge, but it
| was still true when I left in 2018.
| cmrdporcupine wrote:
| 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.
| ggm wrote:
| 2015 was 6 years ago. Google as possibly changed? Doubled in
| size (or more) by staff? has more rigorous approaches? has
| more people capable of meaningfully reviewing code?
| B-Con wrote:
| 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.
| dang wrote:
| Discussed at the time:
|
| _Modern Code Review: A Case Study at Google [pdf]_ -
| https://news.ycombinator.com/item?id=18035548 - Sept 2018 (64
| comments)
| Goog_l_Dude_l wrote:
| 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.
___________________________________________________________________
(page generated 2021-03-31 23:01 UTC)