server: Add EngineTypeDefault, resolve it to Pebble if last used #45512

itsbilal · 2020-02-27T20:37:22Z

Adds a new engine type called EngineTypeDefault, that resolves
at start time to RocksDB, or Pebble if that was last used
to write to the first store. This effectively makes the
--storage-engine flag sticky for future runs where no
engine is specified. If the flag is passed in again, that
specified engine type takes priority over the one used last.

Fixes #42445 .

Release note (cli change): Add new "default" option for
--storage-engine flag that respects the engine used last.

cockroach-teamcity · 2020-02-27T20:37:34Z

This change is

itsbilal · 2020-02-27T20:37:46Z

First commit is #45499 , will rebase it away when that gets merged.

petermattis

Approach looks generally good, though I'm worried about how many places you need to add support for EngineTypeDefault. Were all of those additions actually needed, or should we be making sure that the engine type is "resolved" early and instead error out if we get to server.Start and the engine type is still "default"?

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

pkg/cli/debug.go, line 158 at r2 (raw file):

		db, err = engine.NewPebble(context.Background(), cfg)

	case enginepb.EngineTypeDefault:

I think this should have the same behavior as start. Specifically, we should query pebble.GetVersion. The reason for this is so commands like cockroach debug keys use the same engine that was previously used. That was actually my initial motivation behind this functionality.

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

	case enginepb.EngineTypePebble:
		nodeStartCounter += "pebble."
	case enginepb.EngineTypeDefault:

I think we want to know what engine is actually being used at this point. Will we ever get here with enginepb.EngineTypeDefault still be a valid case?

petermattis

Also, I think you need to update the help text for the --storage-engine flag.

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

itsbilal

Thanks for the review! Walked back some of the Default checks, and added an error at engine creation time, if the engine type is still left at EngineTypeDefault. Test code still continues to treat Default = RocksDB

Also made the help text change.

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

pkg/cli/debug.go, line 158 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I think this should have the same behavior as start. Specifically, we should query pebble.GetVersion. The reason for this is so commands like cockroach debug keys use the same engine that was previously used. That was actually my initial motivation behind this functionality.

Done.

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

Previously, petermattis (Peter Mattis) wrote…

I think we want to know what engine is actually being used at this point. Will we ever get here with enginepb.EngineTypeDefault still be a valid case?

The storage engine value should have been resolved by this point, but just in case it isn't, I added a case for default instead of not reporting anything or errorring out, so we can at least find out something's wrong without erroring out.

itsbilal · 2020-02-29T01:10:03Z

Actually, this turned out to be more involved to implement in test code than I expected at first. We'd basically have to take all instances of MakeConfig in tests, and append a conditional that goes if cfg.StorageEngine == Default { cfg.StorageEngine = EngineTypeRocksDB } (or alternatively we could just set engine.DefaultStorageEngine to EngineTypeRocksDB if it's default right before the MakeConfig calls).

I'd now make the case to go back to the previous, defensive solution where we treat EngineTypeDefault as akin to EngineTypeRocksDB whenever it's encountered, except in start and debug where we make the GetVersion check. It might seem like more threading, but it's less threading than the alternative, plus it feels safer.

petermattis

I'd now make the case to go back to the previous, defensive solution where we treat EngineTypeDefault as akin to EngineTypeRocksDB whenever it's encountered, except in start and debug where we make the GetVersion check. It might seem like more threading, but it's less threading than the alternative, plus it feels safer.

Ack. Not all of my suggestions are good suggestions. I like this proposal.

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

pkg/cli/cliflags/flags.go, line 719 at r3 (raw file):

		Description: `
Storage engine to use for all stores on this cockroach node. Options are rocksdb,
or pebble.

Isn't default an option now too?

pkg/cli/cliflags/flags.go, line 721 at r3 (raw file):

or pebble.

If left unspecified, the storage engine last used to write to the

I'd rephrase this to mention "default": If default is specified. That applies to the other references to "unspecified" as well.

itsbilal

Thanks for the review, this is ready for another look. I've reverted to the default = rocksdb idea. Also added a release note.

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

pkg/cli/cliflags/flags.go, line 719 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Isn't default an option now too?

Done. Yeah, it is, but I was trying to simplify it to just the unspecified case. I like explicitly mentioning default better, so I made the change.

pkg/cli/cliflags/flags.go, line 721 at r3 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I'd rephrase this to mention "default": If default is specified. That applies to the other references to "unspecified" as well.

Done.

petermattis

LGTM modulo a couple of nits.

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

pkg/cli/debug.go, line 154 at r4 (raw file):

			}
		}
	}

This is duplicated with code in runStart, and is just past my threshold where I'd want to extract it into a common method. Something like resolveStorageEngineType(engine.DefaultStorageEngine, dir).

pkg/storage/engine/engine.go, line 560 at r4 (raw file):

	case enginepb.EngineTypeDefault:
		fallthrough
	case enginepb.EngineTypeRocksDB:

Nit: you can write this as case enginepb.EngineTypeDefault, enginepb.EngineTypeDefault:.

pkg/storage/engine/in_mem.go, line 35 at r4 (raw file):

	case enginepb.EngineTypeDefault:
		fallthrough
	case enginepb.EngineTypeRocksDB:

See nit about multi-version case statements.

pkg/storage/engine/temp_engine.go, line 42 at r4 (raw file):

	case enginepb.EngineTypeDefault:
		fallthrough
	case enginepb.EngineTypeRocksDB:

See nit about multi-version case statements.

Adds a new engine type called EngineTypeDefault, that resolves at start time to RocksDB, or Pebble if that was last used to write to the first store. This effectively makes the --storage-engine flag sticky for future runs where no engine is specified. If the flag is passed in again, that specified engine type takes priority over the one used last. Fixes cockroachdb#42445 . Release note (cli change): Add new "default" option for --storage-engine flag that respects the engine used last.

itsbilal

Thanks for the review!

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

pkg/cli/debug.go, line 154 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

This is duplicated with code in runStart, and is just past my threshold where I'd want to extract it into a common method. Something like resolveStorageEngineType(engine.DefaultStorageEngine, dir).

Done.

pkg/storage/engine/in_mem.go, line 35 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

See nit about multi-version case statements.

Done.

pkg/storage/engine/temp_engine.go, line 42 at r4 (raw file):

Previously, petermattis (Peter Mattis) wrote…

See nit about multi-version case statements.

Done.

petermattis

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

itsbilal · 2020-03-02T21:02:55Z

TFTR!

bors r+

craig · 2020-03-02T21:31:54Z

Build failed (retrying...)

GitHub CI (Cockroach)

craig · 2020-03-02T22:47:57Z

Build succeeded

GitHub CI (Cockroach)

itsbilal requested review from knz, petermattis and sumeerbhola February 27, 2020 20:37

itsbilal requested a review from a team as a code owner February 27, 2020 20:37

itsbilal self-assigned this Feb 27, 2020

petermattis reviewed Feb 28, 2020

View reviewed changes

itsbilal force-pushed the sticky-storage-engine branch 2 times, most recently from 499c0ef to 8c1eaf7 Compare February 29, 2020 00:22

itsbilal commented Feb 29, 2020

View reviewed changes

itsbilal force-pushed the sticky-storage-engine branch from 8c1eaf7 to 9a17f1a Compare February 29, 2020 00:39

petermattis reviewed Feb 29, 2020

View reviewed changes

itsbilal force-pushed the sticky-storage-engine branch 2 times, most recently from 8696b29 to e0d59b8 Compare March 2, 2020 17:35

itsbilal commented Mar 2, 2020

View reviewed changes

petermattis approved these changes Mar 2, 2020

View reviewed changes

itsbilal force-pushed the sticky-storage-engine branch from e0d59b8 to 7534af7 Compare March 2, 2020 19:51

itsbilal commented Mar 2, 2020

View reviewed changes

petermattis approved these changes Mar 2, 2020

View reviewed changes

craig bot merged commit 3e56193 into cockroachdb:master Mar 2, 2020

knz added this to To do in DB Server & Security via automation Mar 7, 2020

knz moved this from To do to Done 20.1 in DB Server & Security Mar 7, 2020

jseldess mentioned this pull request Mar 19, 2020

server: Add EngineTypeDefault, resolve it to Pebble if last used cockroachdb/docs#6926

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: Add EngineTypeDefault, resolve it to Pebble if last used #45512

server: Add EngineTypeDefault, resolve it to Pebble if last used #45512

itsbilal commented Feb 27, 2020 •

edited

Loading

cockroach-teamcity commented Feb 27, 2020

itsbilal commented Feb 27, 2020

petermattis left a comment

petermattis left a comment

itsbilal left a comment

itsbilal commented Feb 29, 2020

petermattis left a comment

itsbilal left a comment

petermattis left a comment

itsbilal left a comment

petermattis left a comment

itsbilal commented Mar 2, 2020

craig bot commented Mar 2, 2020

craig bot commented Mar 2, 2020

server: Add EngineTypeDefault, resolve it to Pebble if last used #45512

server: Add EngineTypeDefault, resolve it to Pebble if last used #45512

Conversation

itsbilal commented Feb 27, 2020 • edited Loading

cockroach-teamcity commented Feb 27, 2020

itsbilal commented Feb 27, 2020

petermattis left a comment

Choose a reason for hiding this comment

petermattis left a comment

Choose a reason for hiding this comment

itsbilal left a comment

Choose a reason for hiding this comment

itsbilal commented Feb 29, 2020

petermattis left a comment

Choose a reason for hiding this comment

itsbilal left a comment

Choose a reason for hiding this comment

petermattis left a comment

Choose a reason for hiding this comment

itsbilal left a comment

Choose a reason for hiding this comment

petermattis left a comment

Choose a reason for hiding this comment

itsbilal commented Mar 2, 2020

craig bot commented Mar 2, 2020

Build failed (retrying...)

craig bot commented Mar 2, 2020

Build succeeded

itsbilal commented Feb 27, 2020 •

edited

Loading