Vitess is being used to power many mission-critical production workloads at very large scale.
Moreover, many users deploy directly from the main branch.
It is very important the changes made by contributors do not break any existing workloads.
In order to avoid disruption, the following concerns need to be kept in mind:
Does the change affect any external APIs? If so, make sure the change satisfies the compatibility rules.
Can the change introduce a performance regression? If so, it will be good to measure the impact using benchmarks.
If the change is substantial or is a breaking change, you must publish the proposal as an issue with a title like RFC: Changing behavior of feature xxx. Following this, sufficient time has to be given for others to give feedback. A breaking change must still satisfy the compatibility rules.
New features that affect existing behavior must be introduced "behind a flag". Users will then be encouraged to enable them, but will have the option to fallback to the old behavior if issues are found.
For bigger changes, it is a good idea to start by creating an RFC (Request for Comment) issue - this is where you can discuss the feature and why it's important.
Once that is in place, you can create the PR, as a solution to the problem described in the issue. Separating the need and the solution this way makes discussions easier and more focused.
If you are creating a PR to fix a bug, make sure to create an end-to-end test that fails without your change.
This is the important reproduction case that will make sure this particular bug does not show up again, and that clearly shows on your PR what bug you are fixing.
While you are fixing the bug, it's valuable if you take the time to step back and try to think of other places where this problem could have impacted.
It's often possible to infer that other similar problems in this or other parts of the code base can be prevented.
Some additional points to keep in mind:
Does this change match an existing design / bug?
Is this change going to log too much? (Error logs should only happen when
the component is in bad shape, not because of bad transient state or bad
Does this match our current patterns? Example include RPC patterns,
Retries / Waits / Timeouts patterns using Context, ...
We also recommend that every author look over their code change before committing and ensure that the recommendations below are being followed. This can be done by skimming through git diff --cached just before committing.
Scan the diffs as if you're the reviewer.
Look for files that shouldn't be checked in (temporary/generated files).
Look for temporary code/comments you added while debugging.
Look for inconsistencies in indentation.
Use 2 spaces in everything except Go.
In Go, just use goimports.
Commit message format:
<component>: This is a short description of the change.
If necessary, more sentences follow e.g. to explain the intent of the change, how it fits into the bigger picture or which implications it has (e.g. other parts in the system have to be adapted.)
Sometimes this message can also contain more material for reference e.g. benchmark numbers to justify why the change was implemented in this way.
// Prefer complete sentences when possible.
Leave a space after the comment marker //.
If your reviewer leaves comments, make sure that you address them and then click "Resolve conversation". There should be zero unresolved discussions when the PR merges.
Vitess uses code owners to auto-assign reviewers to a particular PR. If you have been granted membership to the Vitess team, you can add additional reviewers using the right-hand side pull request menu.
During discussions, you can also refer to somebody using the @username syntax and they'll receive an email as well.
If you want to receive notifications even when you aren't mentioned, you can go to the repository page and click Watch.