Versions Compared

Version Old Version 25 New Version Current
Changes made by

David Taylor

Richard Stewart

Saved on

Key

  • This line was added.
  • This line was removed.
  • Formatting was changed.

...

Then, look at our Go (Golang) coding guidelines and the Google Go Code Review guide it links to. These are linked to from the other pages in this wiki, but they're easy to miss. If you haven't written any Go before CockroachDB, you may want to hold off on reviewing the style guide until you've written your first few functions in go.

...

  • build/ Build support scripts. You'll likely only need to interact with [build/builder.sh]. See "Building on Linux" below.

  • c-deps/ Glue to convince our build system to build non-Go dependencies. At the time of writing, "non-Go dependencies" means C or C++ dependencies. The most important of these is RocksDB, our underlying persistent key-value store.

  • cloud/kubernetes/ Kubernetes configuration to auto-launch CockroachDB clusters.

  • docs/ Documentation for CockroachDB developers. See "Internal documentation" below.

  • monitoring/ Configuration to integrate monitoring frameworks, namely Prometheus and Grafana, with CockroachDB. This configuration powers our internal monitoring dashboard as well.

  • pkg/ First-party Go code. See "Internal documentation" below for details.

  • scripts/ Handy shell scripts that aren't part of the build process. You'll likely interact with scripts/gceworker.sh most, which spins up a personal Linux VM for you to develop on in the GCE cloud.

  • vendor/ A Git submodule that contains the right version of our dependent libraries. For many years, the Go answer to dependency management was "write backwards-compatible code." That strategy is as dangerous as it sounds, so since v1.6, Go will automatically load packages in a subdirectory named vendor, if it exists. See build/README.md for details on how we maintain this folder.

Other important repositories

...

Other important repositories

Besides cockroachdb/cockroach, the cockroachdb GitHub organization is home to several other important open-source components of Cockroach:

...

The internal documentation that we do have lives in cockroachdb/cockroach/docs. At the time of writing, most of this documentation covers the high-level architecture of the system. Only a few documents hone in on specifics, and even those only cover the features that were found to cause significant developer frustration. For most first-party packages, you'll need to read the source for usage instructions.

Protip: If you prefer Go-generated HTML documentation to reading the source directly, take a look at godoc.org/github.com/cockroachdb/cockroach.

For For our internal docs, I recommend the following reading order.

...

It's time to fire up your editor and get to work! The other pages on this wiki do a good job of describing the basic development commands you'll need. In short, you'll use make to drive builds.

To produce a cockroach binary:

Code Block
$ make build

To run all the tests in ./pkg/rest/of/path:

Code Block
$ make test PKG=./pkg/rest/of/path

Is there a Go debugger?

Yes.

Is there a Go debugger?

Yes! It's called delve.

You can use delve on its own, but the easiest way is to use it via Goland. Goland is a Jetbrains IDE for Go. It has built in support for delve, and will "just work" if you click the standard debug buttons, set breakpoints, etc.

...

If you're debugging an issue that spans several nodes, you could consider using our distributed tracing. Ask your Roachmate to either walk you through OpenTracing/LightStep or point you at someone who can.

To use a debugger, you will probably have to run ./dev gen go to generate Go code.

A note on adding a new file

You'll notice that all source files in this repository have a license notification at the top. Be sure to copy this license into any files that you create .

...

(adjust the year to be the current year).

When to submit

Code at Cockroach Labs is not about perfection. It's too easy to misinterpret "build it right" as "build it perfectly," but striving for perfection can lead to over-engineered code that took longer than necessary to write. Especially as a startup that moves quickly, building it right often means building the simplest workable solution, then iterating as necessary. What we try to avoid at Cockroach Labs is the quick, dirty, gross hack—but even that can be acceptable to plug an urgent leak.

...

You have a few options for choosing when to submit:

  1. You can open a PR with an initial prototype

...

Especially on your first PR, you can do everything short of opening a GitHub PR by sending someone a link to your local branch. Alternatively, open a PR against your fork. This won't send an email notification to anyone watching this repository, so it's a good way to let your Roachmate (or the lead reviewer on your project). This works well when you have a reviewer in mind and you want to avoid the onslaught of

...

For small changes where the approach seems obvious, you can open a PR with what you believe to be production-ready or near-production-ready code. As you get more experience with how we develop code, you'll find that larger and larger changesets will begin falling into this category.

PRs are assumed to be production-ready (option 3) unless you say otherwise in the PR description. Be sure to note if your PR is a WIP to save your reviewer time.

...

  1. using the “Draft” feature. (“Create Draft PR” in the Github interface). This won't send an email notification to anyone watching this repository.

  2. For small changes where the approach seems obvious, you can open a PR with what you believe to be production-ready or near-production-ready code. As you get more experience with how we develop code, you'll find that larger and larger changesets will begin falling into this category.

PRs are assumed to be production-ready (option 3) unless you create it as “Draft” or say otherwise in the PR description. Be sure to note if your PR is a WIP to save your reviewer time.

If you find yourself with a PR (git diff --stat) that exceeds roughly 500 line of code, it's time to consider splitting the PR in two, or at least introducing multiple commits. This is impossible for some PRs—especially refactors—but "the feature is only partially complete" should never be a reason to balloon a PR.

...

Then, run our suite of linters:

Code Block
$ make./dev lint

This is not a fast command. On my machine, at the time of writing, it takes about a full minute to run. You can instead runYou can instead run

Code Block
$ make lintshort

...

./dev lint --short

which omits the slowest linters.

Where to push

In the interest of branch tidiness, we ask that even contributors with the commit bit avoid pushing directly to this contributors should not push directly to the upstream repository. That deserves to be properly called out:

Protip: don't push to cockroachdb/cockroach directly!

Instead, create yourself a fork on GitHub. This will give you a semi-private personal workspace at github.com/YOUR-HANDLE/cockroach. Code you push to your personal fork is assumed to be a work-in-progress (WIP), dangerously broken, and otherwise unfit for consumption. You can view @bdarnell's WIP code, for example, by browsing the branches on his fork, bdarnell/cockroach.

...

It's far more important to give the commit message a descriptive title and message than getting the branch name right. Note that your branch name does not have to be unique among all branch names in the organization.

Split your change into logical chunks

Be kind to other developers, including your future self, by splitting large commits. Each commit should represent one logical change to the codebase with a descriptive message that explains the rationale for the change.This is explained in our PR organization philosophy.

On your first PR, it's worth re-reading the Git Commit Messages page to ensure your commit messages follow the guidelines.

...

Deciding who should review a PR requires context that you just won't have in your first month. For your starter project, your Roachmate is the obvious choice. For little bugs that you fix along the way, ask your Roachmate or your neighbor if there's someone who's obviously maintaining a particular area of the codebase. (E.g., if you had a question about the code that interfaces with RocksDB, I would immediately direct you to @petermattis.)

If you're unable to get a reviewer recommendation, the "Author" listed at the top of the file may be your best bet. You should check that the hardcoded author agrees with the Git history of the file, as the hardcoded author often gets staleyou can check a recent author from the git log. Use git log FILE to see all commits that have touched a particular file or directory sorted by most recent first, or use git blame FILE to see the last commit that touched a particular line. The GitHub web UI or a Git plugin for your text editor of choice can also get the job done. You might also try scripts/authors.sh FILE, which will show you a list of contributors who have touched the file, sorted by the number of commits by that author that touched the file. If there's a clear winner, ask them for a review; if they're not the right reviewer, they'll suggest someone else who is. In cases that are a bit less clear, you may want to note in a comment on GitHub that you're not confident they're the right reviewer and ask if they can suggest someone better.

If you're still unsure, just ask in chat! You'll usually get a response in under a minute.Slack!

It's important to have a tough reviewer. In the two months I've been here, I've seen a few PRs land that absolutely shouldn't have landed, each time because the person who understood why the PR was a bad idea wasn't looped into the review. There's no shame in this—it happens! I say this merely as a reminder that a tough review is a net positive, as it can save a lot of pain down the road.

Also, note that GitHub allows for both "assignees" and "reviewers" on a pull request. It's not entirely clear what the distinction between these fields is, and we're currently split at Cockroach Labs on what we prefer. Choose the field you like most and move on. Ever since GitHub began auto-suggesting reviewers, there seems to be a slight preference for using the reviewer field instead of the assignee fieldIn general, prefer to assign a reviewer as “reviewer” instead of “assignee”.

Write your PR description

...

Otherwise, something simple like "see commit messages for details" is good enough. On PRs with just one commit, GitHub will automatically include that one commit's message in the description; it's totally fine to leave that in the PR description.

TeamCity

We run our own continuous integration server called TeamCity, which runs unit tests and acceptance tests against all open pull requests.

GitHub displays the status of the latest TeamCity run at the bottom of every pull request. You can click the "Details" link to get insight into a failed build or view real-time status of an in-progress build. Occasionally, a build will fail because of flaky tests. You can verify by running a new build and seeing if the problem disappears; just hit the "Run" button on the top right of the page GitHub links to queue a new build. Less frequently, TeamCity will entirely fail to notice a PR, and GitHub will display "waiting for status to be reported" forever. The "TeamCity Continuous Integration" wiki page describes how to fix this.

...

CI

Monitor your PR statuses for CI jobs labeled “GitHub Actions Essential CI”. Each of these jobs runs a set of checks on your PR, like unit tests, lints, or integration tests. Check out the /wiki/spaces/devinf/pages/3418980354 for tips on how to navigate CI and address failures.

Note: Older release branches (release-23.2 and prior) use TeamCity for CI. If you’re confused, check which status checks are considered to be “Required” by GitHub.

That's it! You're done. Sit back and get ready for the impending code review.

...

Except for the smallest of PRs, many teams eschew GitHub reviews in favor of a third-party app called Reviewable (demo video).

Reviewable has an incredibly dense UI: you could probably master a new musical instrument faster than you could master Reviewable.Still; still, some teams have decided that Reviewable offers a better UI than GitHub for large changes that undergo multiple rounds of feedback. The basic model is the same: a diff of your change where your reviewers can leave inline notes. In Reviewable, unlike GitHub, every comment spawns a new thread of discussion, and the UI will highlight any threads that you haven't acknowledged/responded to.

Reviewable also takes great pains to handle force pushes. When you push a new revision, Reviewable will do its best to match up comment threads to the logically-equivalent location in the new diff. Even if this algorithm fails, Reviewable records each force push as a "revision," and will allow you and your reviewers to track how your PR has evolved as you address review feedback. GitHub’s built-in tooling does not support this and force-pushes will often cause previous iterations of the code to be “lost”.

You won't need to do anything special to enable Reviewable for your PR; one of our bots will be along shortly after the PR is opened to post a link to the review interface.

...

Once you've accumulated a set of changes you need to make to address review feedback, it's time to force-push a new revision. Be sure to amend your prior commits—you don't want to tack on a bunch of fixup commits to address review feedback. If you've split your PR into multiple commits, it can be tricky to ensure your review feedback changes make it into the right commit. A tutorial on how to do so is outside the scope of this document, but the magic keywords to Google for are "git interactive rebase."

...

  • CR, "code review"

  • PR, "pull request"—you probably knew that one.

  • PTAL, "please take another look"

  • RFAL, "ready for another look"—as in, I made some changes, PTAL.

  • LGTM, "looks good to me"—i.e., ship it!

  • woman_scientistImage Modifieddog

    , "science dog"—i.e., I have no idea what this code does.

  • TF[YT]R, "thanks for your/the review"

...