At Cockroach Labs we take code reviewing seriously. This is for a number of reasons, some of which include:
We have an open sourced repository and the quality of our code is a public reflection on our company.
Databases are long-lived pieces of technology, and it's not inconceivable that the code you’re writing now will live within CockroachDB for a decade or more. If spending some additional time before the code is merged ensures that it will be more performant, modifiable or understandable, that time is well spent.
As a result, this document provides some guidance on things that may be commented on as part of your code review. Knowing that these are considerations in the code review process may better prepare you for your first (and subsequent) PRs.
Test cases - First and foremost, the goal of the code review process is to ensure that the code that gets merged into CRDB is correct, understandable and maintainable. While correctness is often difficult to ensure in a code review, it’s quite common for a code reviewer to carefully examine the test cases that are being added/modified to ensure that they adequately cover the code changes, and that they produce the expected result. It’s typical that a code review will result in test case changes or additions.
Go idioms (style) - Code review comments will often touch on Go idioms. As a result, you may want to familiarize yourself with the Go coding guidelines before sending out your first PR. Keep in mind that it’s certainly not expected that your first few PRs will be perfect from a Go idiom perspective, but a cursory read of the guidelines early on in your time at CRL will at the very least serve to better frame some of the code review comments you receive. If you find that a first read of the document is overwhelming, you may want to come back to it after you’ve spent a month or more coding actively in Go, at which point it should be easier to work through.
Nits - In general, there is nothing too small to be commented on in a code review. Reviewers at CRL will commonly comment on addition/removal of whitespace, periods at the end of comments, adhering to line length limits, spelling (even though we have linters to catch the bulk of these errors), etc. In an effort to stress the relative importance of these types of issues, reviewers will commonly preface them with “Nit:” in the review. These are small issues that won’t affect the code’s functionality, but they should still be addressed as part of the review. To prepare for your code review you may want to quickly look over the code right before you send it out with an eye to avoiding nitty mistakes.
Keeping a healthy attitude - It’s natural to feel as though the code review process entails some amount of judgment. In practice however, we’re all just striving to build the best product possible. Try not to feel offended or dismayed by a reviewer pointing out something which makes the code more efficient, maintainable or understandable. It’s often the case that another perspective on a given problem is helpful to break through a mental block and lead to a more elegant solution. While it may seem obvious to take a given approach once it’s pointed out by the reviewer, it may have been far from obvious from your perspective, or considering how you originally approached (or mentally framed) the problem.
Of course, once you become proficient at coding in CRDB, you’ll be relied upon more often to review the code of others. For guidance on how to do that, see Working as a reviewer.