Code Reviews

• Enforcement of code quality and standards - someone else looking at the code may catch errors or make suggestions for improvements
• Learning tool - the person submitting the PR may not be familiar with every class, method, and library in Kafka. Code review is an opportunity to teach about useful libraries, abstractions, gotchas and the inner workings of Kafka.
• Teaching tool - reviewers can often learn something new and cool from the code they review. A new algorithm, a useful library, a solution they haven't thought of.
• Cultural push toward quality - knowing that someone (maybe many someones) will review the code closely encourages contributors to review their own code carefully in order to make the best impression. Countless issues are found in the "one last go-over before sending to review".
• Knowledge spread - by doing reviews we ensure that more than one person is familiar with each part of the code

We'd like to make sure contributors have more or less consistent experience when they submit PRs for review. We also want to make the experience pleasant. Here are some guidelines reviewers can follow to make this happen.

Review Guidelines:

• Are you the right reviewer for this PR? you should be familiar with the code that the PR modifies before you review.
• It is ok to declare intent to review and a time at which you expect to do so. This is better than completely ignoring PRs.
• It is also ok to ask additional people to review the PR because you feel their knowledge is relevant. If you are asked to review and don't think you are the right person - just say so 
• Make sure you fully understand the intent of the PR. Ask high-level questions if needed before diving into the small details. If there is a JIRA, KIP or mailing list discussion related to the PR, make sure you are familiar with them.
• If the code changes configuration, public APIs or the protocol - make sure there is a KIP, that the vote passed and that the code is consistent with the KIP.
• First do no harm: Make sure the code is a net improvement to the code base. Either by adding new functionality, removing unused code, improving performance, refactoring old code to use better abstractions or code practices.
• A large number of comments on technicalities ("line too long") is not fulfilling the intent in reviews. We should add value in addition to what a simple script can provide.
• It is always better to request changes than to ignore or rubber stamp. If there are major problems, feel free to request a major modification to the entire PR rather than annotate each line where problems appear.
• We value consistency - if you are looking at a command line tool, are the parameters similar to other tools we have? do public APIs follow our conventions (no getters and setters for instance)? 
• Review tests as much (or more) than code. Bug fixes should have tests that make sure the bug is fixed, new functionality should have tests that make sure it works as intended, refactoring should have tests to make sure the code continues to function as expected. Make sure the tests actually test things (will the test fail without the bug fix?), make sure they cover various inputs and conditions and include negative tests (do exceptions happen when they should?). If for some reason you must approve code without tests (urgent fix to code that requires refactoring to be testable), at the very least document how you validated the change manually and open a JIRA and follow up later.
• If part of the code is confusing or requires significant thought to understand - ask the author to add a comment. This will help the next guy that will come along. Good comments should explain why the code does things a certain way.
• Needless to mention - code should compile and tests should pass before we LGTM anything.