Code Review Guidelines

Code Review Guidelines

The following is meant to be a set of guidelines, not dogma. Use your best judgement. The goal is twofold, to have a resource for new Engineers to learn about what we collectively care about, and to serve as a reminder while doing development and review.

(See also: how to write PRs)

Advice to the reviewers

To get the best out of a review, we ask the reviewers to take the approach of assuming the code will not work. The purpose of this assumption is to have the implementor prove why the code does work. Being critical of the code does not mean you are being negative. It does mean you are encouraging the implementor to prove you're wrong, for you both to go over the code in detail, which results in more productive code reviews and bugs being detected sooner.

---

Review Checklist:

1. Observability
2. Performance
3. Fault-tolerance
4. Testing
5. Code Clarity/Quality
6. Code Documentation
7. External Documentation
8. Release Notes

More details

Observability

State that might be useful for observing system behavior is exposed in some way, either Erlang console, stats, riak-admin cmd-line, etc. This knowledge is documented somewhere.

Performance

Performance changes are known. Ideally, the code should leave performance as-is, or improve. If the code negatively affects performance, an explanation should be given.

Fault-tolerance

• The system can handle arbitrary process death. Supervisors are used where appropriate. Resource creation/release is understood. If the system can get into a state where it must be restarted (Erlang VM restarted), this is documented. The general assumption should be that restarting a supervision tree should be enough to 'get things back into a working state'.
• If processes are communicating asynchronously, they should be in a proper supervision tree that will crash them all if one of them crashes, or should be monitoring or linked to each other and handling the appropriate 'DOWN' and/or 'EXIT' messages (if process_flag(trap_exit, true) is set) to ensure they don't wait indefinitely on a crashed process.
• Backpressure. More backpressure.

Testing

• The code has sufficient unit, EQC and/or integration testing to feel confident that further changes to this code can be trusted.

• All pre-existing tests that passed before this change continue to pass afterward.

• If the PR fixes a bug, there should be a test (unit/integration/EQC) that proves the code was broken, and is now fixed.

• Ensure that the PR is up-to-date with the branch onto which it is going to be merge. This may require a thumbs retry to verify the merge and a potential request to have the PR rebased against the target branch.

Code Clarity/Quality

• Functionality is not repeated.
• Functions are small. A function is small enough when it's at the granularity you'd like to play with at a REPL.
• Code does not use manual recursion when a higher-order function would both suffice and be more clear to the next person looking at the code.
• All top-level functions have type specifications.
  • this is a great chance to run dialyzer and xref
• The code uses idiomatic Erlang.
• The code takes advantage of OTP behaviors and idioms.

Code Documentation

• All exported functions have clear documentation to their usage.
• Every function has at least a single sentence comment describing functionality.
• New modules have a module description.
• EDoc for "publicly used" libraries like riak_core, optional for libraries used predominantly by Riak.

External Documentation

Reasonable additions/updates to staging for external documentation has been made. This may be done in concert with a CSE. This also serves to help other employees get up to speed with the feature changes.

Release Notes

A short description of what changes were made with the pull request, and why. This should, to the best of your abilities, be able to be directly incorporated into public release notes. This should not be "did stuff to things, for reasons."