*: Remove RocksDB #55509

itsbilal · 2020-10-13T15:03:07Z

As in title. Removes RocksDB and snappy, from the c-deps directory,
and updates tests and roachtests to reflect a Pebble-only world. Libroach
is slimmed to just the one method (DumpThreadStacks) that is still necessary.

As this is a massive change, I've tried to make it as easy to review as possible by
splitting changes in different commits. The first commit just updates temp file locking,
the second commit deletes RocksDB-specific code paths and updates the few remaining
RocksDB-only tests to use equivalent Pebble code paths, the fourth commit has
Makefile changes, and the last commit is a plain file deletion in c-deps.

This is ready for review, though the following relatively-minor tasks are still TODO:

Resolve merge conflicts
Pass lint
Bring back thread stack dumps as they may be necessary for inspecting other CGo code

cockroach-teamcity · 2020-10-13T15:03:22Z

This change is

otan · 2020-10-13T15:06:59Z

what's the reason we still need c-deps/jemalloc and c-deps/cryptopp?
edit: seems like jemalloc is needed for Bring back jemalloc thread stack dumps as they may be necessary for inspecting other CGo code

otan · 2020-10-13T15:09:42Z

pkg/cmd/roachtest/engine_switch.go

@@ -140,6 +140,7 @@ func registerEngineSwitch(r *testRegistry) {
 	r.Add(testSpec{
 		Name:       fmt.Sprintf("engine/switch/nodes=%d", n),
 		Owner:      OwnerStorage,
+		Skip:       "rocksdb removed in 21.1",


since roachtest is always based on master, doesn't this block all invocations?

It does. I'm told @jlinder+@adityamaru are working to address #51897 shortly.

knz · 2020-10-13T15:27:51Z

jemalloc is needed for all the cgo code

irfansharif · 2020-10-13T17:27:10Z

We'll need to do something here as well:

cockroach/pkg/cmd/roachtest/versionupgrade.go

Lines 507 to 508 in 69f0fb5

    
           c.Run(ctx, c.All(), cockroach, "debug", "rocksdb", "--db={store-dir}", 
        
           	"checkpoint", "--checkpoint_dir={store-dir}/"+name)

pkg/cli/cliflags/flags.go

pkg/cli/debug.go

itsbilal · 2020-10-14T21:05:06Z

This is ready for a look. I could split the first 3 commits into a separate PR if that makes them easier to review, but the last two are fairly mechanical anyway so I don't expect much code reviewing discussion on those.

Only big piece that's still TODO is that thread stack dumps are disabled now as they are a casualty of the libroach deletion. If they're still worthwhile to keep, I could bring back just that part of libroach, or see if there's a way we can get a view of threads from Go. Thoughts welcome.

otan · 2020-10-14T21:13:37Z

Only big piece that's still TODO is that thread stack dumps are disabled now as they are a casualty of the libroach deletion. If they're still worthwhile to keep, I could bring back just that part of libroach, or see if there's a way we can get a view of threads from Go. Thoughts welcome.

fwiw, libroach was not hard to bazel-ify so i'm not opposed to keeping that part of libroach.

pkg/cmd/roachtest/versionupgrade.go

knz · 2020-10-15T08:25:18Z

fwiw, libroach was not hard to bazel-ify so i'm not opposed to keeping that part of libroach.

As long as we still have cgo code (and we do), it seems to be a good thing to keep?

itsbilal · 2020-10-16T21:03:06Z

Sorry for the noise! But this is ready for another look. I've brought libroach back, just for thread stack dumping. Also fixed many tests, and working my way through the last minor build/CI-related issues. version-upgrade fixtures generation will be fixed when cockroachdb/pebble#962 lands, which should happen before this lands.

petermattis

I didn't see any *.rej files remaining in any of the commits. Reviewable shows some as they existed in an old revision, but I deleted them after and amended the commits.

Ah, perhaps they were deleted and I didn't notice that. Carry on.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @itsbilal, @jbowens, @jlinder, @otan, @pbardea, and @sumeerbhola)

.gitmodules, line 7 at r22 (raw file):

	path = c-deps/jemalloc
	url = https://github.com/cockroachdb/jemalloc.git
[submodule "c-deps/cryptopp"]

Is cryptopp still needed? I thought it was only used for RocksDB encryption-at-rest.

pkg/cmd/roachtest/inconsistency.go, line 25 at r22 (raw file):

		Name:       fmt.Sprintf("inconsistency"),
		Owner:      OwnerKV,
		Skip:       "Uses RocksDB put command; unskip when that's bypassed",

Is the intention to fix this soon? I wouldn't want to leave this test skipped for too long. Perhaps file an issue.

pkg/kv/kvserver/store.go, line 855 at r22 (raw file):

	//
	// TODO(bilal): Delete this and all compactor-related code, now that Pebble is
	// the only storage engine.

File an issue unless you plan to do this immediately.

pkg/server/debug/server.go, line 183 at r22 (raw file):

	})

	mux.HandleFunc("/debug/threads", func(w http.ResponseWriter, req *http.Request) {

Should this be left in? I think we want to keep the thread-stack dumps.

pkg/storage/pebble_iterator.go, line 298 at r22 (raw file):

// NoSplitSpans.
func isValidSplitKey(key roachpb.Key, noSplitSpans []roachpb.Span) bool {
	if key.Equal(keys.Meta2KeyMax) {

Is this a real bug fix? Does it need to be backported to 20.2 and 20.1?

pkg/storage/enginepb/engine.proto, line 25 at r22 (raw file):

  EngineTypeDefault = 0;
  // Denotes RocksDB as the underlying storage engine type.
  EngineTypeRocksDB = 1;

Is it valid to use reserved = 1 in an enum?

Previously, we used RocksDB's file locking mechanism for temp directory locking. As RocksDB is being removed from the codebase, move to using Pebble's syscall-based mechanism. Release note: None.

itsbilal

TFTRs! Again, sorry for the force-push noise; lots of subtle build issues that are only happening on TC but not locally. But most of them should be behind us now.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @jbowens, @jlinder, @otan, @pbardea, @petermattis, and @sumeerbhola)

.gitmodules, line 7 at r22 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is cryptopp still needed? I thought it was only used for RocksDB encryption-at-rest.

It's not. @otan pointed this out too. I'll fix this right away in a follow-up PR as this one is already large as-is.

pkg/cmd/roachtest/inconsistency.go, line 25 at r22 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is the intention to fix this soon? I wouldn't want to leave this test skipped for too long. Perhaps file an issue.

I'll file an issue, thanks for the reminder!

pkg/kv/kvserver/store.go, line 855 at r22 (raw file):

Previously, petermattis (Peter Mattis) wrote…

File an issue unless you plan to do this immediately.

Filing an issue.

pkg/server/debug/server.go, line 183 at r22 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Should this be left in? I think we want to keep the thread-stack dumps.

Done. Added it back; deleted it by mistake.

pkg/storage/pebble_iterator.go, line 298 at r22 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is this a real bug fix? Does it need to be backported to 20.2 and 20.1?

It's not an issue as the old code is a bit more strict (i.e. it would advise against splitting at start keys of all spans, new code allows splitting at all start keys except for the Meta2 one). Might be worth getting a second opinion from KV ( @irfansharif ?), but basically the new disallowed-set is a subset of the old one, so we're actually less strict now.

pkg/storage/enginepb/engine.proto, line 25 at r22 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Is it valid to use reserved = 1 in an enum?

Done.

This change deletes all RocksDB specific Go code, and updates all tests that called RocksDB-specific methods to call their Pebble versions instead. This is a prerequisite for deleting RocksDB from the codebase. Release note (general change): RocksDB can no longer be used as the storage engine. Passing in --storage-engine=rocksdb will return an error.

As --storage-engine=rocksdb no longer works as a command line argument, update the help text for that flag to reflect its nature. Also skip engine/switch, and inconsistency roachtests as they use RocksDB-only functionality. Release note (cli change): Update --storage-engine help text to reflext RocksDB deletion.

This change updates the Makefile to not build snappy, and rocksdb. Libroach is built without those two libraries as well. Release note: None.

This change is a simple file deletion of: c-deps/{rocksdb,snappy}{,-rebuild} Release note: None

This change deletes all of Libroach except for the methods necessary to implement DBDumpThreadStacks. Also moves the Go-side code to pkg/storage/stacks.go. Release note: None.

petermattis

modulo questions about the isValidSplitKey change. I think that change is good, just wondering if it should be pulled into a separate PR.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @jbowens, @jlinder, @otan, @pbardea, and @sumeerbhola)

.gitmodules, line 7 at r22 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

It's not. @otan pointed this out too. I'll fix this right away in a follow-up PR as this one is already large as-is.

Ack. 👍

pkg/storage/pebble_iterator.go, line 298 at r22 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

It's not an issue as the old code is a bit more strict (i.e. it would advise against splitting at start keys of all spans, new code allows splitting at all start keys except for the Meta2 one). Might be worth getting a second opinion from KV ( @irfansharif ?), but basically the new disallowed-set is a subset of the old one, so we're actually less strict now.

Ack. I'd like a second opinion. This feels like something that could get pulled out into a separate commit/PR for easy backporting. I agree that the change to use ProperlyContainsKey is a loosening of the split point logic, but the disallowing of splits at Meta2KeyMax is a strengthening of that logic.

itsbilal

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @jbowens, @jlinder, @otan, @pbardea, and @sumeerbhola)

pkg/storage/pebble_iterator.go, line 298 at r22 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ack. I'd like a second opinion. This feels like something that could get pulled out into a separate commit/PR for easy backporting. I agree that the change to use ProperlyContainsKey is a loosening of the split point logic, but the disallowing of splits at Meta2KeyMax is a strengthening of that logic.

Yep, and the key point is that Meta2KeyMax is already one of the start keys in noSplitSpans, so it would have been disallowed in both the old code (through ContainsKey), and in the new code (through the special case). The loosening is effectively only with all the other disallowed spans.

petermattis

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @irfansharif, @jbowens, @jlinder, @otan, @pbardea, and @sumeerbhola)

pkg/storage/pebble_iterator.go, line 298 at r22 (raw file):

Previously, itsbilal (Bilal Akhtar) wrote…

Yep, and the key point is that Meta2KeyMax is already one of the start keys in noSplitSpans, so it would have been disallowed in both the old code (through ContainsKey), and in the new code (through the special case). The loosening is effectively only with all the other disallowed spans.

Ah, I was missing that Meta2KeyMax was a start key of one of the spans in noSplitSpans. So we were always disabling splits at Meta2KeyMax, but with the change to use ProperlyContainsKey below we wouldn't be.

itsbilal · 2020-10-22T12:42:25Z

Time to roll with it then. Thanks for all the reviews!

bors r+

craig · 2020-10-22T13:21:52Z

Build succeeded:

GitHub CI (Cockroach)

libcryptopp was only used for RocksDB encryption-at-rest. Now that RocksDB is deleted (cockroachdb#55509), we can remove libcryptopp too. Release note: None.

55858: *: Remove cryptopp from c-deps, stop building it r=petermattis a=itsbilal libcryptopp was only used for RocksDB encryption-at-rest. Now that RocksDB is deleted (#55509), we can remove libcryptopp too. Release note: None. Co-authored-by: Bilal Akhtar <[email protected]>

We don't need it anymore. We originally introduced it to support tests in libroach (cockroachdb#20594), which has since been pared down to something much simpler (cockroachdb#55509, just DBDumpThreadStacks). Release note: None

55900: c-deps: remove googletest r=irfansharif a=irfansharif We don't need it anymore. We originally introduced it to support tests in libroach (#20594), which has since been pared down to something much simpler (#55509, just DBDumpThreadStacks). Release note: None Co-authored-by: irfan sharif <[email protected]>

itsbilal requested review from jbowens, otan, petermattis and a team October 13, 2020 15:03

itsbilal requested a review from a team as a code owner October 13, 2020 15:03

itsbilal self-assigned this Oct 13, 2020

itsbilal requested review from pbardea and removed request for a team October 13, 2020 15:03

otan reviewed Oct 13, 2020

View reviewed changes

irfansharif reviewed Oct 13, 2020

View reviewed changes

pkg/cli/cliflags/flags.go Show resolved Hide resolved

pkg/cli/debug.go Show resolved Hide resolved

itsbilal force-pushed the delet-rocksdb branch from 26b888b to 48eb390 Compare October 14, 2020 14:13

blathers-crl bot requested a review from otan October 14, 2020 14:13

itsbilal force-pushed the delet-rocksdb branch 3 times, most recently from 9b999e6 to ecf4b11 Compare October 14, 2020 21:02

irfansharif reviewed Oct 14, 2020

View reviewed changes

pkg/cmd/roachtest/versionupgrade.go Outdated Show resolved Hide resolved

itsbilal force-pushed the delet-rocksdb branch 3 times, most recently from 730fc4c to 9a51b02 Compare October 16, 2020 21:01

itsbilal force-pushed the delet-rocksdb branch 3 times, most recently from 2cb0b0a to 1069ae6 Compare October 20, 2020 14:53

itsbilal force-pushed the delet-rocksdb branch 2 times, most recently from a66a971 to f9a4880 Compare October 21, 2020 19:38

petermattis reviewed Oct 21, 2020

View reviewed changes

storage: Use vfs.FS.Lock for temp file locking instead of RocksDB

5ddbc35

Previously, we used RocksDB's file locking mechanism for temp directory locking. As RocksDB is being removed from the codebase, move to using Pebble's syscall-based mechanism. Release note: None.

itsbilal force-pushed the delet-rocksdb branch from f9a4880 to 26253ee Compare October 21, 2020 20:38

itsbilal commented Oct 21, 2020

View reviewed changes

itsbilal added 5 commits October 21, 2020 16:46

*: Update makefile to not build rocksdb, snappy

036d9ff

This change updates the Makefile to not build snappy, and rocksdb. Libroach is built without those two libraries as well. Release note: None.

*: Delete RocksDB, snappy from c-deps

fe629c0

This change is a simple file deletion of: c-deps/{rocksdb,snappy}{,-rebuild} Release note: None

libroach: Slim libroach to only have the DBDumpThreadStacks method

afe5d48

This change deletes all of Libroach except for the methods necessary to implement DBDumpThreadStacks. Also moves the Go-side code to pkg/storage/stacks.go. Release note: None.

itsbilal force-pushed the delet-rocksdb branch from 26253ee to afe5d48 Compare October 21, 2020 20:46

petermattis approved these changes Oct 21, 2020

View reviewed changes

itsbilal commented Oct 21, 2020

View reviewed changes

This was referenced Oct 21, 2020

storage: Delete compactor code, metrics, and settings #55839

Closed

storage: Unskip inconsistency roachtest #55840

Closed

petermattis approved these changes Oct 22, 2020

View reviewed changes

craig bot merged commit fdeb78b into cockroachdb:master Oct 22, 2020

itsbilal mentioned this pull request Oct 22, 2020

*: Remove cryptopp from c-deps, stop building it #55858

Merged

irfansharif mentioned this pull request Oct 23, 2020

c-deps: remove googletest #55900

Merged

irfansharif mentioned this pull request Oct 23, 2020

*: bazel-ify cockroachdb #55687

Merged

itsbilal mentioned this pull request Dec 1, 2020

storage, bulk: Transition backup code from RocksDBSstFileReader to sstable.Reader #42375

Closed

jseldess mentioned this pull request Dec 9, 2020

Remove RocksDB cockroachdb/docs#9080

Closed

nicktrav mentioned this pull request Nov 17, 2021

pkg/storage: remove rocksdb from error string #72847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

*: Remove RocksDB #55509

*: Remove RocksDB #55509

itsbilal commented Oct 13, 2020 •

edited

Loading

cockroach-teamcity commented Oct 13, 2020

otan commented Oct 13, 2020 •

edited

Loading

otan Oct 13, 2020

irfansharif Oct 13, 2020

knz commented Oct 13, 2020

irfansharif commented Oct 13, 2020

itsbilal commented Oct 14, 2020

otan commented Oct 14, 2020

knz commented Oct 15, 2020

itsbilal commented Oct 16, 2020

petermattis left a comment

itsbilal left a comment

petermattis left a comment

itsbilal left a comment

petermattis left a comment

itsbilal commented Oct 22, 2020

craig bot commented Oct 22, 2020

*: Remove RocksDB #55509

*: Remove RocksDB #55509

Conversation

itsbilal commented Oct 13, 2020 • edited Loading

cockroach-teamcity commented Oct 13, 2020

otan commented Oct 13, 2020 • edited Loading

otan Oct 13, 2020

Choose a reason for hiding this comment

irfansharif Oct 13, 2020

Choose a reason for hiding this comment

knz commented Oct 13, 2020

irfansharif commented Oct 13, 2020

itsbilal commented Oct 14, 2020

otan commented Oct 14, 2020

knz commented Oct 15, 2020

itsbilal commented Oct 16, 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 Oct 22, 2020

craig bot commented Oct 22, 2020

itsbilal commented Oct 13, 2020 •

edited

Loading

otan commented Oct 13, 2020 •

edited

Loading