cli: Add flag to choose pebble as storage engine

[itsbilal]

This change adds a --storage-engine CLI flag, which has options
rocksdb and pebble. Specifying pebble ensures all stores created
on this node, including the temp engine, use pebble instead of rocksdb.

This is a per-node flag instead of a per-store flag to eliminate cases
where we have to maintain two caches, one for rocksdb instances and
one for pebble instances.

Also remove --temp-engine flag, which is now unnecessary.

Release note (cli change): Add command-line argument --storage-engine
to specify type of storage engine to use. Options are rocksdb (default)
and pebble.

[cockroach-teamcity]

This change is 

[itsbilal]

Dependency graph of related PRs:

#41199 ---------------------  #41452  --------------------------- #41455
(pebble engine)       \      (pebble MVCC scanner)              (MVCC test refactor)
                       \        
                        \
                         ----- #41453
                            (CLI changes)
                            (this PR)

[petermattis]

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @itsbilal, @petermattis, and @sumeerbhola)

---

pkg/base/config.go, line 153 at r2 (raw file):

// Type implements the pflag.Value interface.
func (e *EngineType) Type() string { return "string" }

I'm not sure what Type() is used for, but I think it is supposed to distinguish from other flag types. Perhaps this should be "engine".

---

pkg/server/config.go, line 496 at r2 (raw file):

				// TODO(itsbilal): Tune these options, and allow them to be overridden
				// in the spec (similar to the existing spec.RocksDBOptions and others).
				pebbleOpts := &pebble.Options{

You don't seem to be specifying cache. Am I missing where that is done?

[knz]

Not reviewing the code yet, I have two questions and a remark:

1. how did you motivate the choice to make this flag global, instead of per-store? We document that a node can use multiple stores, is there a reason not to enable different stores from using different storage engines? (Like we already enable in-memory stores as a separate store type)

2. You mention that it only works for non-temporary stores. So that means that temporary stores still use rocksdb? What was the rationale for that? (The rationale should be detailed in the commit message)

3. This is a user-facing change (and a prominent one at that) and thus should be detailed in a release note.

[petermattis]

how did you motivate the choice to make this flag global, instead of per-store? We document that a node can use multiple stores, is there a reason not to enable different stores from using different storage engines? (Like we already enable in-memory stores as a separate store type)

A per-store flag is possible, but then we need to figure out how to allocate the block cache between Pebble and RocksDB. My preference is to avoid this complication and just make the flag apply to all stores. Actually trying to share the block cache between Pebble and RocksDB is a non-starter due to technical difficulty.

You mention that it only works for non-temporary stores. So that means that temporary stores still use rocksdb? What was the rationale for that? (The rationale should be detailed in the commit message)

Oh, good point. We should make this apply to temporary stores too!

[knz]

A per-store flag is possible, but then we need to figure out how to allocate the block cache between Pebble and RocksDB. My preference is to avoid this complication and just make the flag apply to all stores. Actually trying to share the block cache between Pebble and RocksDB is a non-starter due to technical difficulty

This is a good explanation. It should be detailed in the commit message too.

[itsbilal]

how did you motivate the choice to make this flag global, instead of per-store? We document that a node can use multiple stores, is there a reason not to enable different stores from using different storage engines? (Like we already enable in-memory stores as a separate store type)

Peter has already answered this, but yes, the goal is to eliminate double-caching. I had originally implemented a per-store flag but was advised to go with this approach.

There's a separate flag for temp stores that's already in master and 19.2, --temp-engine. Given we were already creating a new cache for that in rocksdb (and would be doing the same in pebble), we don't lose out much if the temp and non-temp storage engines don't match.

There's probably more value in having more fine-grained control of each store that way, right? Instead of having --storage-engine override whatever was specified for --temp-engine (or eliminating the latter altogether). I'm fine with it either way, just want to know what would be the better way forward.

[petermattis]

There's probably more value in having more fine-grained control of each store that way, right? Instead of having --storage-engine override whatever was specified for --temp-engine (or eliminating the latter altogether). I'm fine with it either way, just want to know what would be the better way forward.

I had forgotten about --temp-engine. I think we should have one flag that controls all use of Pebble vs RocksDB.

[itsbilal]

Updated commit message, added a release note, fixed cache instantiation (discussed in #41199), and removed --temp-storage. PTAL!

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajkr, @petermattis, and @sumeerbhola)

---

pkg/base/config.go, line 153 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'm not sure what Type() is used for, but I think it is supposed to distinguish from other flag types. Perhaps this should be "engine".

It seems like we expect it to be a primitive type everywhere else. Other similar Type() functions on CLI argument types seem to return either string or int or something of that nature.

---

pkg/server/config.go, line 496 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

You don't seem to be specifying cache. Am I missing where that is done?

Done. Cache is instantiated here now.

[knz]

Updated commit message, added a release note

💯

[petermattis]

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajkr, @itsbilal, and @sumeerbhola)

---

pkg/base/config.go, line 153 at r2 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

It seems like we expect it to be a primitive type everywhere else. Other similar Type() functions on CLI argument types seem to return either string or int or something of that nature.

It doesn't have to be a primitive type. The spf13/pflag library we use internally uses types like intSlice and stringArray. I'm not quite sure what the purpose of this is, though. The docs provide no clue and it only seems to be used for special formatting. Fine to leave as "string".

[itsbilal]

Thanks for the review! Did a no-change rebase, going to merge when CI is green. Though the flag will practically be useless until #41452 lands.

[petermattis]

Thanks for the review! Did a no-change rebase, going to merge when CI is green. Though the flag will practically be useless until #41452 lands.

You might want to hold off on merging, or perhaps add a commit which disables support for pebble, otherwise you'll be fielding questions about why pebble doesn't work.

[itsbilal]

Makes sense. I'll hold off on merging until #41452 lands then. Thanks!

[itsbilal]

bors r+

[craig]

Build succeeded

• GitHub CI (Cockroach)