-
Notifications
You must be signed in to change notification settings - Fork 5.4k
Review and Commit guidelines
Presto is a community driven project staffed entirely by volunteers. Contributors, reviewers, and committers are all donating their time to effect positive change, and it is important to keep that in mind no matter what role you happen to occupy.
The end goal of Presto’s contribution process is that changes should be appropriate for Presto and adequately reviewed and tested. How we get there is handled on a case by case basis as not all changes require the same level of review. Individual committers decide when something is ready (or veto what is not ready), the community provides feedback and calibration to committers, and the committer pool polices itself.
Only committers have binding votes.
A single +1 binding vote is required for something to land and anyone can leave a -1 vote. A binding -1 vote accompanied with a valid technical reason stops a change forever until the technical reason is addressed and the voter withdraws their veto. A non-binding -1 vote should always be carefully considered and treated like a binding vote unless there is an argument for overriding it.
A committer cannot +1 their own code modifications. All code modifications require independent review.
A +1 vote on a code modification is an indication that the voter has reviewed and tested the change and found it to meet community standards.
A +1 vote is left by approving a PR. A -1 vote must be left as a comment. Requesting changes via the Github pull request UI does not constitute a -1 vote. If changes have been requested then committers should make an effort to make sure they are addressed before committing.
Fractional votes are advisory only and can serve as a form of abstention or as part of discussion.
Code contributions are effectively always up for vote once a pull request is created. Votes on code modifications should last long enough that interested parties subscribed to the repository have a chance to review and comment. How long this should be depends on the scope, impact, risk, and how long the change has been under discussion. Calendar considerations like weekends and holidays should also be taken into account. Three business days is a good starting point for substantial changes.
A one line log change to the Presto CLI does not require multiple reviewers and the reviewer doesn’t need to be someone with extensive experience in that area. A several thousand line change to the planner that touches the SPI and has a wide impact might require two or more reviewers some of which have extensive experience in different areas.
Whether a reviewer is a committer or not makes no difference. The end goal is that the reviewers are capable of reviewing the proposed changes, and that adequate review takes place.
Committers are ultimately responsible for everything they commit. When a committer commits something they didn’t review themselves they are expressing that they trust that the reviewers and contributors have done work that meets community standards.
If a committer is unsure about a change they should review themselves, ask questions, or review the associated tests/test output to see if something is safe to merge. This is an opportunity for committers to mentor and propagate our culture and community standards, and it is an important part of the committer role.
Committers are under no obligation to commit something if they aren’t satisfied with it. It is the job of the contributor to get their contribution up to community standards. Committers may help them to speed things along (especially with new contributors), but if a contributor can’t find a single committer willing to commit their change as is then the proposed change is the problem.
That said it’s important to do everything we can to get changes over the finish line and allow people to scratch their own itch using Presto. Make the path forward clear even if you can’t make anyone take it, and don’t say no without giving a reason.
The end goal of our usage of commit messages, pull requests, and GitHub issues is to create a readable commit history at a high enough level that it can be read by a user or developer who is not already familiar with each individual change. The Git log of commits between releases should end up producing a serviceable change log.
Commit messages should always follow Chris Beams commit guidelines.
Avoid several substantially similar commits achieving an aggregate goal. For example several instances of “Do XYZ to class ABC”. It’s fine to do this in a pull request for review purposes, but it creates an unreadable commit history on master.
Attribution is required for all code copied from other projects. Rewriting all or most of the code does not eliminate the contributions of the original authors of the copied code.
Commits cherry-picked from other projects should have their original author field preserved if the patch applies cleanly. Use the name and email address as it appears in git-log in the original repository if the original author has signed the CLA. If the original author has not signed the CLA or the patch has to be rewritten then they should be added as the co-author and the person contributing the commits should list themselves as the author. Co-authors can be added using GitHub's multiple author functionality. You don't need to add yourself as co-author if you are committing yourself. When merging together multiple external commits include as many co-authors as necessary to capture all the contributors.
Be sure to have a line separating the first line of the commit message otherwise GitHub won't parse it, and no lines following the co-author lines. You also need to include an accurate email address in <>. When it works there should be an icon for the co-author in the GH UI listing commits.
When committing locally you can use --author="First Last [email protected]" where you cut and paste the author information from the original commit. You can use interactive rebase to edit existing commits and correct author information on your branch.
Fix OOM caused by foo in bar
Foo was pack ratting ByteBuffers causing OOM.
Cherry-pick of https://github.com/foo/bar/pull/123/commits/c2bf3b025ff456921c4c5ff4bcb814740e823061 (https://github.com/foo/bar/pull/123/commits/c2bf3b025ff456921c4c5ff4bcb814740e823061)
Co-authored-by: Foo Bar <[email protected]>
Cherry-pick and co-author lines are only necessary when cherry-picking a commit. The co-author tag is only necessary if you are not committing yourself. The co-author tag won't work if there isn't a line of separation between it and the commit message.
The PR message shall also contain the cherry-pick information of the original PR.
Fix OOM caused by foo in bar
Foo was pack ratting ByteBuffers causing OOM.
Cherry-pick of https://github.com/foo/bar/pull/123 (https://github.com/foo/bar/pull/123)
Co-authored-by: Foo Bar <[email protected]>
If you are adding a new function, language feature, or other similarly user-facing functionality, documentation should be included as part of the pull request that adds the new feature (can be a separate commit in the PR).