A study of Google's code review tooling (Critique) • Hacker News Board

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

#141

Post by zeroonetwothree » Mon, Dec 04, 2023, 6:42 PM UTC

joatmon-snoo wrote: post #133

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 t…

At Facebook there is no OWNERS so if you make an API change you can just update all the code yourself at once.

It makes it much easier to rapidly iterate and improve shared libraries.

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

#142

Post by johnnyanmac » Mon, Dec 04, 2023, 6:42 PM UTC

marcosdumay wrote: post #75

BytesAndGears wrote: post #44

Earlier quoted context omitted.

I like your thought process here, but let me propose an alternative. I’m going to assume that we’re talking about Senior->Senior level reviews here. Junior engineers should obviously always get more guidance. But for Seniors, the alternative is: Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow”…

I've received plenty of negative feedback on the internet for this (and up to now, discarded all), but I really don't think acceptance review is the correct time for guidance. Yes, juniors need guidance. That means you must read their code, and talk to them about it. They also need real-world feedback. They do really need you not managing all of the interaction they have with the real world. Now, what you can not man…

So, when is the right time?

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

#143

Post by cmrdporcupine » Mon, Dec 04, 2023, 6:44 PM UTC

sparcpile wrote: post #135

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 u…

I assume that Critque is Grerrit with a bunch of Google-specific changes.

Not even close. I have another comment where I get into some details, but, no, three's no overlap beyond the fact that Gerrit pulled some UI and workflow things from Critique (and Mondrian before that, the tool that predated Critique)

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

#144

Post by cmrdporcupine » Mon, Dec 04, 2023, 6:45 PM UTC

nappy-doo wrote: post #56

jasonlotito wrote: post #46

Earlier quoted context omitted.

Yeah, it seems like Gerrit with lots of Google-specific stuff. Not surprised. Used Gerrit for 12 years, and loved it.

Gerrit is pretty crap compared to critique. It has a workflow that works for Android, but critique is really much better.

The essential thing is that Gerrit is a swiss army knife review tool for git, whereas Critique is able to be consistent and fluid because it only has to worry about working with the standard workflow in Piper/CitC (and now fig I guess)

I agree Critique is much nicer, but mostly because it's more consistent and doesn't have to deal with all the oddities of git.

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

#145

Post by johnnyanmac » Mon, Dec 04, 2023, 6:45 PM UTC

withinboredom wrote: post #105

BytesAndGears wrote: post #44

Earlier quoted context omitted.

I like your thought process here, but let me propose an alternative. I’m going to assume that we’re talking about Senior->Senior level reviews here. Junior engineers should obviously always get more guidance. But for Seniors, the alternative is: Be extremely picky for PRs from new hires, so that they do things the way that your company does them. Make sure they put files in the right places, and adhere to the “flow”…

> 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…

No, the word is called "alignment"

>an arrangement of groups or forces in relation to one another

If you're being harassed in PRs the peer review system has failed. It's a place to make sure everyone is on the same page, not judge someone for how they code.

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

#146

Post by SpaceManNabs » Mon, Dec 04, 2023, 6:45 PM UTC

tgsovlerkhgsel wrote: post #80

SpaceManNabs wrote: post #31

What is wrong with bazel?

Bazel is a build system, not a code review system. Entirely different thing.

You should open the article. The very first content in the article is someone praising critique and also saying they wont use bazel.

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

#147

Post by Reubend » Mon, Dec 04, 2023, 6:49 PM UTC

zdragnar wrote: post #86

cgearhart wrote: post #62

Earlier quoted context omitted.

This kind of greedy optimization approach is fine if all you’re just trying to maximize velocity, but it starts showing cracks on a long enough timeline in sufficiently large or complex projects. Speed without alignment on direction leads to ugly, tangled messes. As with most quality issues, the key is to try and surface quality problems as early as possible. Finding a bug in prod is worse than finding it in PR is wo…

> In my team we prefix nitpicky comments explicitly with “nit:” and it’s up to the author to decide what to do with it This is different from what the OP is talking about. I've worked at places where staff engineers seem to have been rated on number of comments left on PRs... they were typically somewhere between nit picky and useless, with the occasional person directly contradicting feedback they'd given in a previ…

> I've worked at places where staff engineers seem to have been rated on number of comments left on PRs

> Oh, and you had to address every one of them before getting approval.

This just sounds horrible.

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

#148

Post by ravenstine » Mon, Dec 04, 2023, 6:52 PM UTC

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…

The fact that your comment is currently at the top of the thread tells me that a lot of people go through this, yet this is interesting to me because past HN discussions have suggested said experience to be an exception. In my own experience, blocking (or simply not approving) PRs over nits, out-of-scope improvements, and mere personal preferences is the norm. What's worse is when PRs get blocked because a person doesn't like the pattern the author is using even though it's already one of many existing patterns and there's no explicit guideline anywhere. I find this happens a lot when lead and staff engineers make decisions on what's "considered harmful" but don't actually broadcast that decision in an effective way.

What I try to do with my code reviews is prefix my comments with "Non-blocking suggestion:" or "Minor question:" so that the other person is more likely to know that a particular piece of feedback doesn't come with the expectation of needing to be addressed.

Otherwise, if the code works, has tests, and isn't objectively incorrect, I try to give an approval. If you don't like something about it, there should be room in the process to make improvements. The idea that "mistakes won't actually get fixed later" is a cop out. If your team can't fix mistakes, then you should turn in your "engineer" titles.

Also, when programmers do things that others on their team disagree with, it almost always comes down to a breakdown in communication. Junior developers are a bit of a different story, but senior developers not knowing how to write code that the rest of the team approves of means that people aren't communicating, and it probably means that your system is so complex that it's unreasonably difficult to determine the proper approach in the first place.

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

#149

Post by rapfaria » Mon, Dec 04, 2023, 6:54 PM UTC

> Unresolved comments represent action items for the change author to definitely address. These can be marked as “resolved” by the code author themselves when they reply to the comment.

I've always told the team that whoever comments is the one that marks it as resolved (we use github). Otherwise they just gotta go and open every single comment again, and sometimes the code author doesn't even it address on big changes.

This assumes the reviewer will comeback later to re-review it, of course.

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

#150

Post by johnnyanmac » Mon, Dec 04, 2023, 6:55 PM UTC

jrockway wrote: post #70

iwontberude wrote: post #59

Earlier quoted context omitted.

If you link peoples ability to use their computer by how quickly they can click "Approve" on a PR, it will definitely lead to a break down in review quality and beat you at your own personal records of latency and MTTR.

I'm sure there would be negative effects, but at least things would move forward quickly. This is the worst system except for all the others.

Sounds like you work best on a small team if that's your focus. Medoum-large ones already have a proven product (or proven experience). You don't focus on blazing gast iterations at that stage unless you are in early R&D or something evergreen.

And well, if you're primary motivation is Financial: okay, you're not being paid to move quickly. Or if you hope to move up the ladder quickly you gotta deal with the office politics. Which includes delays in communication. React to those lapses as you feel is appropriate to your goals and personality.