Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

A study of Google's code review tooling (Critique)

engineercodex.substack.com

Post reply on HN
Print view View original

131–140 of 302 posts

Re: A study of Google's code review tooling (Critique)

#131

Post by arp242 »

shadowgovt wrote: post #103
arp242 wrote: post #60

Earlier quoted context omitted.

But "another team is relying on not to move" is an objective point, right? Things like "I find this code very hard to follow, and I think it could be made easier" is also objective, and even "I don't understand what this variable name means, and I think it could be clearer". I once names a function mkdir(). This created a directory tree. In the review it was called "obscure" so it became createDir(). Then someone poi…

> Things like "I find this code very hard to follow, and I think it could be made easier" is also objective The "I" in that sentence suggests this should be considered subjective. And that's I think the cleave-point between gating and non-gating: "Other people have already agreed on this" vs. "In the moment, I, a single code-reviewer, think this name could be improved."

It's not an exact science and there's a grey area of course, but what I have in mind is mostly code that's just needlessly confusing in ways I think very few would disagree with. I can't really think of a specific non-trivial example at the moment, but just like natural language I do think some code can objectively "hard to follow", even if that's somewhat vague and not strictly defined.

Other factors are if I'm the primary or one of the primary maintainers, the standing of the other person (also a maintainer or one-time contributor from another team), and things like that.

Re: A study of Google's code review tooling (Critique)

#132

Post by withinboredom »

DandyDev wrote: post #126
withinboredom wrote: post #109

Earlier quoted context omitted.

No, it's not and there is a ton of literature (even entire books) written on why it isn't the time to bring this up during a review. For starters, it results in a waste of time for literally everyone involved: 1. The person writing has to rewrite it (probably). 2. The reviewer could have sat down with the person before a single line of code was written. 3. Anyone else reviewing just wasted their time because it will…

The alternative is a big design upfront?

Generally, it goes something like this imaginary Slack convo:

Me: I think we are getting events from SQS out of order and that’s why we are seeing some weird synchronization issues. What do you think about sending events to the regular queue and a FIFO queue at the same time and comparing them?

Team: How would that work?

Me: Since we are using a single threaded consumer here, I think we can simply override the transport class and instead of pulling from one queue per transport instance, we set up a feature flag allowing us to pull from two, compare the events and if they are different, alerting us in the logs.

Team: Sounds good!

You don’t need a full design spec, just agreement from the team on the approach. There won’t be any surprises in the review.

Re: A study of Google's code review tooling (Critique)

#133

Post by joatmon-snoo »

tallowen wrote: post #13

I’m always surprised to see the totality of support for this workflow. My biggest gripes were: - Owners - Readability review (or really the 18month queue to get Java/python readability) Although seemingly innocuous, this made maintaining internal libraries very challenging. There was no way to update every call site of your library in an efficient way. Tools like Rosie were added on top so that you could shard your P…

As someone who left in 2021:

- OWNERS is absolutely a necessary and important thing, and yes it sucked when it made finding an approver hard, but the point of OWNERS was to optimize for _local_ ownership. (For non-Google folks: think CODEOWNERS files, but hierarchical/recursive, so approvers in /OWNERS, a/OWNERS, a/b/OWNERS, and a/b/c/OWNERS can approve changes anywhere in a/b/c/...)

- I joined in 2017 and it never took 18 months to get readability. If it took you 18 months, it was because you spent 2 weeks writing code in one of those languages and then didn't keep it up. I worked through the stats on this too- by the time I left in 2021, there was virtually no delay to enter the readability process for Java and it was maybe a few weeks for Python.

> There was no way to update every call site of your library in an efficient way.

Does FB have something better here?

G had sufficiently many tools for this, IMO: csearch+grep+sed for the easy changes, Refaster/clang-tidy for more complex stuff (admittedly C++ AST matchers are black magic that few people on even the C++ teams understood). Although I do wish I had known about Comby before I'd left.

Rosie - the tool for executing large-scale changes, i.e. if you made a change to 1K+ files (often 10K+ or 100K+), you needed a way to break the change up into multiple PRs - was also an absolutely critical part of this. I never found the approval process Byzantine - undocumented, perhaps, but remarkably streamlined considering that LSCs meant making simultaneous changes to code in Ads, Android, Cloud, Geo, Search, Technical Infrastructure, YouTube, or however Google breaks down the engineering these days.

Re: A study of Google's code review tooling (Critique)

#134

Post by bvrmn »

Topic article basically shows screenshots with UI quite similar to Gerrit. It would be really great to get even a small glimpse of details from ex-googlers instead of "Critique is so much better than Gerrit".

We have a kind of big repo for a single product with multiple teams working on it. For my company I've configured a few Gerrit QoL things:

* Submit rules with different sets of required reviewers per directory.

* Keep review labels on trivial rebases.

* CI could be triggered manually by comment in a review.

* Submit requires CI verified label.

Gerrit has a nice query language to search for reviews.

On top of that Gerrit provides patch-oriented workflow backing slick review process and keeps history clean.

Re: A study of Google's code review tooling (Critique)

#135

Post by sparcpile »

yx827ha wrote: post #12

If you want something similar, check out Gerrit: https://www.gerritcodereview.com/ It's open source and used by Android and Chrome.

It looks like Critque is a branch of Gerrit. The user interface is similar. I assume that Critque is Grerrit with a bunch of Google-specific changes.

Gerrit itself is an interesting review tool. It uses Git references to manage the review changeset before it is merged into the parent branch.

I used it on a project that used Redmine for issue tracking and Gerrit for the git repo and review tool. It took a bit to get used to how to git to push changes to a Gerrit review so we ended up using Git extensions to manage that.

Re: A study of Google's code review tooling (Critique)

#136

Post by cmrdporcupine »

bvrmn wrote: post #134

Topic article basically shows screenshots with UI quite similar to Gerrit. It would be really great to get even a small glimpse of details from ex-googlers instead of "Critique is so much better than Gerrit". We have a kind of big repo for a single product with multiple teams working on it. For my company I've configured a few Gerrit QoL things: * Submit rules with different sets of required reviewers per directory.…

I used both Gerrit and Critique. There are some similarities, and some configurations of Gerritt can be similar. But...

Critique is not git based. And so a lot of the complexity around rebasing & commit management is just not there. Nor is the stuff about patch-oriented workflow, etc. It's a much simpler mental model (central monorepo built on a fork of Perforce, though they've added some DVCS stuff with 'fig' onto that since), and consistent across the whole giant monorepo. Some of the pleasantness comes from that. Some of the decisions and twiddly knobs in Gerrit probably come from Google teams wanting some of the Critique (or Mondrian before it) workflow, but on top of Git.

The UI in Critique is somehow a little more consistent and fluid and attractive. Many of the keybindings in Gerrit come straight from Critique, and having keybindings is one of the things that makes both of them much nicer to work with than the crap in GitHub, etc.

Gerrit is also extremely configurable, as you've pointed out.

The query language is one thing they have in common with each other, and it's very nice.

In my last year at Google I went back to working in Google3&Critique after working in Gerrit & git for some years, and I quite enjoyed it.

Re: A study of Google's code review tooling (Critique)

#137

Post by withinboredom »

notnullorvoid wrote: post #125
withinboredom wrote: post #105

Earlier quoted context omitted.

> Be extremely picky for PRs from new hires, so that they do things the way that your company does them. I think the word you are looking for is "hazing": > haze (v): force (a new or potential recruit to the military or a university fraternity) to perform strenuous, humiliating, or dangerous tasks. I had this at my current job. I almost quit because it was downright humiliating to get called an "idiot" in so many nic…

They are not suggesting hazing. I don't doubt that you experienced hazing. Being vigilant with new hires to assure assignment on code quality and design is not hazing though. If someone has legitimate concerns about the design decisions made, then they should voice them. However if they are refusing to adhere to guidelines, simply because they dislike the approach then that's being overly problematic.

It’s literally the definition of hazing. But instead of being asked to jump in a pool, naked, while snowing, you are asked to build things a new hire has no business building. Then nit-picked for not knowing things. Literally set up for failure.

A better solution is to actually sit with them while they build a feature, show them around the code, and answer questions. You know, treat them like a team member instead of making them prove their mettle.

Re: A study of Google's code review tooling (Critique)

#138

Post by johnnyanmac »

withinboredom wrote: post #109
thiht wrote: post #98

Earlier quoted context omitted.

> A PR is waaaaay too late to bring up No. That’s literally the point of a review.

No, it's not and there is a ton of literature (even entire books) written on why it isn't the time to bring this up during a review. For starters, it results in a waste of time for literally everyone involved: 1. The person writing has to rewrite it (probably). 2. The reviewer could have sat down with the person before a single line of code was written. 3. Anyone else reviewing just wasted their time because it will…

1. That's the point. It's not up to standard

2. They could but they usually don't. It's fine to sit down and make sure they understand some esoteric structure within a system I owj I'm not going to sit down with someone else to make sure they use for each loops unless it's a very new junior (i wouldn't block that in most code reviews anyway unless the code was legitimately painful to read, but every company is different).

3. IME there may be multiple reviewers but one primary reviewer. Half the time the non-primary ones either focus on "is this code going to break your work flow?" or are simply there for bookkeeping and isn't the approval you're looking for (i.e. A lead on a review that already talked about the plans but doesn't have the knowledge to review that specific module). Anymore than 1 primary reviewer probably needed a smaller PR if possible.

Re: A study of Google's code review tooling (Critique)

#139

Post by notnullorvoid »

jrockway wrote: post #53
throwawaaarrgh wrote: post #14

> Developers need to make progress; overly difficult reviews can discourage future improvements. This, to me, is the most important aspect of code review: get out of the way . If you are putting in nit picks, asking for changes that aren't related to a bug or bad architecture choice, etc, then get out of the way. If you are holding up the approval because you want someone to rewrite the code the way you would have wr…

To me the biggest time sink in PR reviews is getting the reviewer to open the code review. If I ever take over the world, I'd make a new OS that locks all activity whenever you get a code review and doesn't let you switch applications until you click "approve" or "request changes". I find it really crazy how long people take to do reviews. I have Github connected to Slack; if someone needs my review I get a message a…

PR review is an async process, you can't expect your teammates to drop what they are working on in order to review a PR. This is something you can bring up in your stand-up or message channel if the PR has been sitting without a review then kindly ask "hey X, and Y can you give this a review today". Or if it actually is urgent then message someone directly and say it's urgent, and offer to hop on a call to go through the review together so the feedback can be immediate.

Actual priority items are always owned by the whole team, not an individual.

Re: A study of Google's code review tooling (Critique)

#140

Post by hot_gril »

uoaei wrote: post #122
hot_gril wrote: post #118

Earlier quoted context omitted.

Long-term project health rests on bigger things like API design, database choices, or service layout.

Hopefully these are in-scope when it comes to accepting or rejecting PRs and are not just considered incidental to whatever the stated purpose of the PR is. What you listed appears to be more fundamental system design concerns so hopefully they are candidates for further review.

It's usually out of scope by the time you're writing a CL, which is fine because there's design review. The issue is about priorities. Nitpicky code review can be very expensive, and I'm not convinced it brings noticeable benefit.

I've seen team-quarters wasted due to bad large-scale decisions. Sometimes the truth doesn't come out until the the real thing is in production, and all these extra burdens make it harder to change course. I think people are starting to get this because the amount of code nitpicking or caring about small things in general has gone way down.

Post reply on HN