Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update docs to note Pebble is the default for 20.2 #8833

Merged
merged 1 commit into from
Nov 9, 2020

Conversation

rmloveland
Copy link
Contributor

Fixes #7562 #8392 #8595

Summary of changes:

  • Update almost all mentions of "RocksDB" across the 20.2 docs to say
    "the storage engine" as often as possible, and "Pebble" where we must
    because it is relevant.

  • In particular, update the architecture's storage layer docs to mention
    Pebble and link to the Github repo and announcement blog post.

@cockroach-teamcity
Copy link
Member

This change is Reviewable

@rmloveland rmloveland requested review from itsbilal and removed request for itsbilal November 4, 2020 22:58
@rmloveland
Copy link
Contributor Author

Additional questions that may inform this PR:

  1. In the 'Memory Leak' section of Troubleshoot Cluster Setup,
    where it discusses suspected memory leaks, can we now delete
    the section on CGo allocated memory from the Suspected memory
    leak
    section?
    • (possibly incorrect assumption: RocksDB was the main reason
      for the mention of CGo allocated memory)
  2. Has the running_status field reported by SHOW JOBS been
    updated to say 'Pebble compaction'? Alternatively, is it
    staying 'RocksDB compaction' but just referring to Pebble now?
  3. Are the names of the various rocksdb.* cluster settings
    staying the same, but now controlling Pebble settings?
  4. Are the various metrics described on the Custom Chart Debug
    Page
    (on my machine available at
    http://localhost:8888/#/debug/chart) still going to to be called
    rocksdb.*? Presumably yes, but they will now be referring to
    things happening in Pebble?

Copy link

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the 'Memory Leak' section of Troubleshoot Cluster Setup, where it discusses suspected memory leaks, can we now delete the section on CGo allocated memory from the Suspected memory leak section?

we should add a statement that the Pebble block cache is also allocated in C.

(possibly incorrect assumption: RocksDB was the main reason for the mention of CGo allocated memory)

Has the running_status field reported by SHOW JOBS been updated to say 'Pebble compaction'? Alternatively, is it staying 'RocksDB compaction' but just referring to Pebble now?

Yes, it is referring to Pebble if they are running Pebble

Are the names of the various rocksdb.* cluster settings staying the same, but now controlling Pebble settings?
Not sure they're actually documented explicitly anywhere, but we do reference them in the TPC-C instructions docs

I believe you mean things like rocksdb.min_wal_sync_interval. Yes, they are now controlling Pebble.
I am not sure if there are any that were exclusively for RocksDB @itsbilal may remember.

Are the various metrics described on the Custom Chart Debug Page (on my machine available at http://localhost:8888/#/debug/chart) still going to to be called rocksdb.*? Presumably yes, but they will now be referring to things happening in Pebble?

Correct. We don't want to change the metric names and break existing users.

:lgtm:

Reviewed 8 of 8 files at r1.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rmloveland)


v20.2/cockroach-start.md, line 130 at r1 (raw file):

Pebble differs from RocksDB in that it is written in Go and implements a subset of RocksDB's large feature set.

how about adding (to encourage the use):
..., and contains optimizations that benefit CockroachDB.


v20.2/known-limitations.md, line 197 at r1 (raw file):

### Large index keys can impair performance

The use of tables with very large primary or secondary index keys (>32KB) can result in excessive memory usage. Specifically, if the primary or secondary index key is larger than 32KB the default indexing scheme for SSTables breaks down and causes the index to be excessively large. The index is pinned in memory by default for performance.

... for storage engine SSTables ...


v20.2/architecture/life-of-a-distributed-transaction.md, line 122 at r1 (raw file):

### Reads from the storage layer

All operations (including writes) begin by reading from the local instance of the Pebble storage engine to check for write intents for the operation's key. We talk much more about [write intents in the transaction layer of the CockroachDB architecture](transaction-layer.html#write-intents), which is worth reading, but a simplified explanation is that these are provisional, uncommitted writes that express that some other concurrent transaction plans to write a value to the key.

Pebble is just the default, but someone can be using RocksDB. So I think we should just say "... instance of the storage engine ..."


v20.2/architecture/life-of-a-distributed-transaction.md, line 161 at r1 (raw file):

CockroachDB leverages Raft as its consensus protocol. If you aren't familiar with it, we recommend checking out the details about [how CockroachDB leverages Raft](https://www.cockroachlabs.com/docs/v2.1/architecture/replication-layer.html#raft), as well as [learning more about how the protocol works at large](http://thesecretlivesofdata.com/raft/).

In terms of executing transactions, the Raft leader receives proposed Raft commands from the leaseholder. Each Raft command is a write that is used to represent an atomic state change of the underlying key-value pairs stored in the Pebble storage engine.

ditto


v20.2/architecture/life-of-a-distributed-transaction.md, line 175 at r1 (raw file):

## On the way back up

Now that we have followed an operation all the way down from the SQL client to the Pebble storage engine, we can pretty quickly cover what happens on the way back up (i.e., when generating a response to the client).

ditto


v20.2/architecture/replication-layer.md, line 150 at r1 (raw file):

Committed Raft commands are written to the Raft log and ultimately stored on disk through the storage layer.

The leaseholder serves reads from its [Pebble](https://github.com/cockroachdb/pebble) instance, which is in the storage layer.

ditto


v20.2/architecture/storage-layer.md, line 18 at r1 (raw file):

Each CockroachDB node contains at least one `store`, specified when the node starts, which is where the `cockroach` process reads and writes its data on disk.

This data is stored as key-value pairs on disk using the Pebble storage engine, which is treated primarily as a black-box API. Internally, each store contains two instances of Pebble:

we should say that it is the default and could be RocksDB if the user explicitly chose it.


v20.2/architecture/storage-layer.md, line 35 at r1 (raw file):

### Pebble

CockroachDB uses Pebble––an embedded key-value store with a RocksDB-compatible API developed by Cockroach Labs––to read and write data to disk. You can find more information about it on the [Pebble GitHub page](https://github.com/cockroachdb/pebble) or in the blog post [Introducing Pebble: A RocksDB Inspired Key-Value Store Written in Go](https://www.cockroachlabs.com/blog/pebble-rocksdb-kv-store/).

... uses Pebble by default ...

We should continue to have a short section on RocksDB


v20.2/architecture/storage-layer.md, line 41 at r1 (raw file):

- Key-value store, which makes mapping to our key-value layer simple
- Atomic write batches and snapshots, which give us a subset of transactions
- It is developed by Cockroach Labs engineers

we can add another bullet like

@rmloveland rmloveland force-pushed the 20201103-pebble-is-the-default branch from c9c75b2 to 0761f96 Compare November 6, 2020 19:52
Copy link
Contributor Author

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the review Sumeer ! I think my updates address your feedback. Please let me know if you think any additional changes are needed!

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @sumeerbhola)


v20.2/cockroach-start.md, line 130 at r1 (raw file):

Previously, sumeerbhola wrote…
Pebble differs from RocksDB in that it is written in Go and implements a subset of RocksDB's large feature set.

how about adding (to encourage the use):
..., and contains optimizations that benefit CockroachDB.

Updated to add!


v20.2/known-limitations.md, line 197 at r1 (raw file):

Previously, sumeerbhola wrote…

... for storage engine SSTables ...

Updated - and made "storage engine" a link to the description we just updated on the cockroach start page.


v20.2/architecture/life-of-a-distributed-transaction.md, line 122 at r1 (raw file):

Previously, sumeerbhola wrote…

Pebble is just the default, but someone can be using RocksDB. So I think we should just say "... instance of the storage engine ..."

Makes sense - updated to be generic. I made this first mention of "storage engine" be a link to the 'Storage Layer' page of the architecture docs.


v20.2/architecture/life-of-a-distributed-transaction.md, line 161 at r1 (raw file):

Previously, sumeerbhola wrote…

ditto

Done!


v20.2/architecture/life-of-a-distributed-transaction.md, line 175 at r1 (raw file):

Previously, sumeerbhola wrote…

ditto

Done!


v20.2/architecture/replication-layer.md, line 150 at r1 (raw file):

Previously, sumeerbhola wrote…

ditto

Updated to say simply "The leaseholder serves reads from the storage layer."


v20.2/architecture/storage-layer.md, line 18 at r1 (raw file):

Previously, sumeerbhola wrote…

we should say that it is the default and could be RocksDB if the user explicitly chose it.

Added a little section that says that we use Pebble by default, but users can select RocksDB. It repeats the info from cockroach start but I think it's probably worth it for folks who are here reading about the storage layer.


v20.2/architecture/storage-layer.md, line 35 at r1 (raw file):

Previously, sumeerbhola wrote…

... uses Pebble by default ...

We should continue to have a short section on RocksDB

Edited to say "Pebble by default" with a link to the options on cockroach start

Added back pretty much the whole RocksDB section following the Pebble section, but edited to say it's not the default (again with link to cockroach start flags), so users who opt for it don't lose information they might still need.


v20.2/architecture/storage-layer.md, line 41 at r1 (raw file):

Previously, sumeerbhola wrote…

we can add another bullet like

Updated to add! PTAL

Copy link
Contributor

@ericharmeling ericharmeling left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great Rich!

Just a few minor grammar nits. After those are fixed (if you choose to do so), ship it!

Reviewed 6 of 6 files at r2.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @rmloveland and @sumeerbhola)


v20.2/architecture/storage-layer.md, line 44 at r2 (raw file):

really 

Not sure we need "really" here.


v20.2/architecture/storage-layer.md, line 46 at r2 (raw file):

 Key-value store,

It is a key-value store,


v20.2/architecture/storage-layer.md, line 47 at r2 (raw file):

Atomic write batches and snapshots

It implements atomic write batches and snapshots

(or something like that)


v20.2/architecture/storage-layer.md, line 55 at r2 (raw file):

it

RocksDB

(the current phrasing grammatically implies that you are choosing "CockroachDB")

Copy link

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 6 files at r2.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @rmloveland and @sumeerbhola)

Fixes #7562 #8392 #8595.

Summary of changes:

- Update almost all mentions of "RocksDB" across the 20.2 docs to say
  "the storage engine" as often as possible, and mention Pebble where we
  need to because it is relevant.  Additionally, we mention some
  benefits of Pebble w.r.t. optimizations it makes possible in
  CockroachDB.  Where necessary, we continue to mention RockDB since it
  will remain an option in v20.2.

- In particular, update the architecture's storage layer docs to mention
  Pebble and link to the Github repo and announcement blog post, and the
  the bulk import optimization blog post that highlights interesting
  work on Pebble.
@rmloveland rmloveland force-pushed the 20201103-pebble-is-the-default branch from 0761f96 to 73f4e75 Compare November 9, 2020 17:30
Copy link
Contributor Author

@rmloveland rmloveland left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ericharmeling, @rmloveland, and @sumeerbhola)


v20.2/architecture/storage-layer.md, line 44 at r2 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…
really 

Not sure we need "really" here.

What about really really really

ok, deleted :-)


v20.2/architecture/storage-layer.md, line 46 at r2 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…
 Key-value store,

It is a key-value store,

Fixed!


v20.2/architecture/storage-layer.md, line 47 at r2 (raw file):

Previously, ericharmeling (Eric Harmeling) wrote…
Atomic write batches and snapshots

It implements atomic write batches and snapshots

(or something like that)

Updated!

@rmloveland rmloveland merged commit 342d47c into master Nov 9, 2020
@rmloveland rmloveland deleted the 20201103-pebble-is-the-default branch November 9, 2020 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

storage: change default engine type from RocksDB to Pebble
4 participants