If you are making your first contributions, please review the instructions for making a contribution.
This document is an attempt to capture a shared set of values, practices, and learnings. Even though a lot of this may seem obvious, there is value in establishing a more formal reference: to come to an agreed upon set of values, to help new contributors ramp-up in the project, to foster discussion, etc.
Many companies rely on Mesos as a foundational layer of their software infrastructure and it is imperative that we ship high quality, robust code. We aim to foster a culture where we can trust and rely upon the work of the community.
The following are some of the aspirational principles and practices that guide us:
- We value craftsmanship: code should be easy to read and understand, should be written with high attention to detail, and should be consistent in its style. Code is written for humans to read and maintain!
- We value resiliency: our system must be highly-available, stable, and backwards-compatible. We must consider the implications of failures.
- We value accountability: we own and support our software, and are accountable for improving it, fixing issues, and learning from our mistakes.
- We value design and code review: review helps us maintain a high quality system architecture and high quality code, it also helps us mentor new contributors, learn to collaborate more effectively, and reduce the amount of mistakes.
- We value automated testing: automated testing allows us to iterate and refactor in our large codebase, while verifying correctness and preventing regression.
We've found that small, independent, incremental changes are much easier to review thoroughly, and much easier to get committed. Here are some tips to consider before sending review requests:
- Use up front discussion: Surprising a reviewer is likely to lead to wasted effort if the overall design needs changing. Try to get a reviewer on the same page before sending them a review request.
- Think about how to break up your patch. Did you make any changes that could could be reviewed in independent patches? We use
support/post-reviews.py, which makes it easy to create "chains" of reviews based on commits. Become familiar with interactive rebasing to split up a commits. - Provide context for the change: Make the motivations for the change clear in the review request, so the reviewer is not left guessing.
- Follow the style guide and the style of code around you.
- Be patient, thoughtful, and respectful: when providing (a) feedback on reviews and (b) commenting on feedback. This is not meant to be a sparring match! A prerequisite to being a good reviewee is acknowledging that 'writing is hard'! The reviewee should give the reviewer the benefit of the doubt that the reviewer has a real concern even if they can't articulate it well. And the reviewee should be prepared to anticipate all of the reviewers concerns and be thoughtful about why they decided to do something a particular way (especially if it deviates from a standard in the codebase). On the flip side, the reviewer should not use the fact that 'writing is hard' as a crutch for giving hasty feedback. The reviewer should invest time and energy trying to explain their concerns clearly and carefully. It's a two-way street!
- Resolve issues before committing: we tend to give a 'ship it' even when we think there are some issues that need correcting. Sometimes a particular issue will require more discussion and sometimes we take that discussion to IM or IRC or emails to expedite the process. It's important, however, that we publicly capture any collaborations/discussions that happened in those means. Making the discussion public also helps involve others that would have liked to get involved but weren't invited. a. If the reviewer and reviewee are having problems resolving a particular "confrontational" issue then both parties should consider communicating directly or asking another reviewer to participate. We're all here to build the highest quality code possible, and we should leverage one another to do so. b. When an issue is "Dropped" by the reviewee, the expectation is that there is a reply to the issue indicating why it was dropped. A silent "Drop" is very ambiguous. c. If an issue is marked as "Resolved", the expectation is that the diff has been updated in accordance (more or less) with the reviewer's comment. If there are significant changes, a reply to the issue with a comment is greatly appreciated.
- Be explicit about asking for more feedback: feel free to update reviews as often as you like but recognize that in many cases it's ambiguous for reviewers when a review is back into a "ready" state so the reviewee should feel free to ping the reviewers via email when they're ready.
- For reviewees, wait for a 'ship it': We often use the original developer or current "custodian" of some particular code to be the reviewer and add others to get more feedback or as an FYI. Feel free to explicitly call out that you're adding some people just as an FYI in the 'Description'. It's worth mentioning that we often reach out directly to one another about a particular change/feature before we even start coding. A little context goes a long way to streamlining the review process.
- For reviewers, be thourough when giving a 'ship it': understand that reviewing requires a substantial investment of time and focus: a. You are expected to understand and support code that you're giving a 'ship it' to. b. You are expected to be accountable for the quality of the code you've given a 'ship it' to.
Only committers have the ability to commit your change, so make sure you've worked with one during the review. If you are a committer, here are some tips for committing changes:
- Follow the format of commit messages:
a. Be clear and explicit in the commit message.
b. Include the link to the review (this will be done automatically if using
support/post-reviews.pyfor your own changes andsupport/apply-review.shfor committing changes from others). Be sure to clean up the commit message when pulling in changes from others. c. Use 72 character columns. Note that we don't always have a 50 character or less summary because that restriction tends to cause people to write poorly. - Never ever commit a merge: always rebase instead, as appropriate. Likewise, never 'force push'.
- Don't break the build: we support Linux and Mac OS X, however not all configurations are being tested in Jenkins, so be aware of that. Also, pay attention to the Jenkins review bot if it flags a review as breaking the build. Note that if you do break the build, the fixes are often small and inconsequential so don't worry about going through a review cycle for that, just fix things (but don't take that as a license to "wait until the build fails to fix things").
You can become a committer by doing things that the community values, in a way that the community values. More to come here!