You should what you're code reviewing for to get value out of it. No one's perfect (even senior folks) and having a second set of eyes on code is a good way to make sure someone's thought through the implications.

... but Google had production outages because of code review 🧵 https://twitter.com/skamille/status/1198060881168535552
So back in the day, you needed two kinds of code review to check in code: an approval from someone who was listed as an owner of the code and a readability review. If you or your other reviewer had readability in that language, you didn't need a separate review.

If you didn't...
This sometimes caused some unfortunate situations. For example, the RPC system that literally everything in production used to talk to everything else wasn't exactly staffed at some point and I was the only one who would take calls and debug after 5pm PST.
So when the RPC system had a problem and I was called/messaged (I didn't have a pager, everyone just knew my number) by some frantic SREs, I would debug it with them, fix the issue... and then need the readability of my code checked before I could check it in and fix production.
In order to get readability in each of the two languages I needed to fix the RPC system, I had to write new, clean code in those languages. But my day job was infrastructure security and my 20% job was the RPC system, and the code I was working with was... quirky. So no.
So it's really late, some major Google production system or product is keeling over, I have a fix for the SREs to deploy... and I can't check it in. We would have to track down someone with readability. Sometimes this would take a while and the system would stay broken until then
Eventually two of the readability reviewers took pity on me when I explained that we were literally unable to fix major production outages because of my lack of readability review. They reviewed the quirky RPC system code, made some readability tweaks, passed me, problem solved.
(Well, first they suggested I do a 20% project to get clean code. I pointed out that this would mean not doing my 20% project which was "being literally the only staffing on the Java RPC system and 1/2 of the staffing on the C++ side" and that no one would enjoy the results.)
(If you're going to ask why the system which was used for literally every thing in the Google production system to talk to literally every other thing was staffed by two people in their copious free time, that's a whole other story. And that was eventually fixed in ~2009.)
Point being: having someone check for logic errors and other things you missed is a freaking fantastic idea. (Remind me to tell you about the time I took down a few search and ads clusters some time. Oops.) But, like all protective measures, make sure they line up with needs.
Either make sure people are set up to funnel nicely through the protective measures, even in an emergency, or provide a break-glass button with auditing down the line.
You can follow @LeaKissner.
Tip: mention @twtextapp on a Twitter thread with the keyword “unroll” to get a link to it.

Latest Threads Unrolled: