storage/engine: add Pebble support to ExportToSst#41945
storage/engine: add Pebble support to ExportToSst#41945hueypark wants to merge 1 commit intocockroachdb:masterfrom
Conversation
Most of these commits are from [engine: Add ExportToSst method with all logic moved to C++](cockroachdb@75f1314). It is not currently testable (PASS on my local) and can be checked when [roachtest: add flag (or env var) to request pebble cockroachdb#4162](cockroachdb#41620) is completed. Fixes cockroachdb#41739 Release note: None
|
This change is |
petermattis
left a comment
There was a problem hiding this comment.
Thanks for tackling this, @hueypark. I think your approach works, but I have a suggestion for a different structuring. Rather than the approach taken here, I was envisioning adding Engine.ExportToSst. Then the existing call to engine.ExportToSst could be transformed to e.ExportToSst. The Pebble implementation would live in either storage/engine/pebble.go or storage/engine/pebble_export.go.
You'll have to move MVCCIncrementalIterator from ccl/storageccl to storage/engine and I might suggest some cleanups when that is done. I think I'd break this up into 2 PRs: moving MVCCIncrementalIterator and then a follow-on PR which implements the Pebble version of ExportToSst (which you've done in this PR).
Lastly, both MVCCIncrementalIterator and ExportToSst deserve to be unit tested in isolation. It would be fantastic if you could add such testing as you do this work, though I won't be strict about this.
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @hueypark)
pkg/ccl/storageccl/export.go, line 194 at r1 (raw file):
switch e.(type) { case *engine.RocksDB, *engine.RocksDBReadOnly:
Type switches like this which exist outside of the storage/engine package are a code smell. I'd like to get rid of all of them. See my other comment which suggests moving this all to storage/engine. If that is done, you won't need to export RocksDBReadOnly which is desirable.
hueypark
left a comment
There was a problem hiding this comment.
Thanks for the review. I will back with 2 PRs.
Reviewable status:
pkg/ccl/storageccl/export.go, line 194 at r1 (raw file):
Previously, petermattis (Peter Mattis) wrote…
Type switches like this which exist outside of the
storage/enginepackage are a code smell. I'd like to get rid of all of them. See my other comment which suggests moving this all tostorage/engine. If that is done, you won't need to exportRocksDBReadOnlywhich is desirable.
Ok.
This is prerequisites for [storage/engine: add Pebble support to ExportToSst](cockroachdb#41739). Recovered some tests from [engine: Add ExportToSst method with all logic moved to C++](cockroachdb@75f1314). See cockroachdb#41945 Release note: None
This is prerequisites for [storage/engine: add Pebble support to ExportToSst](cockroachdb#41739). Recovered some tests from [engine: Add ExportToSst method with all logic moved to C++](cockroachdb@75f1314). See cockroachdb#41945 Release note: None
This is prerequisites for [storage/engine: add Pebble support to ExportToSst](cockroachdb#41739). Recovered some tests from [engine: Add ExportToSst method with all logic moved to C++](cockroachdb@75f1314). See cockroachdb#41945 Release note: None
41914: sqlmigrations: don't run baked-in migrations r=andreimatei a=andreimatei Before this patch, the migration runner had a mechanism by which it attempted to not run migrations that were included in the cluster bootstrap schema. That mechanism was weak. A migration could be marked as includedInBootstrap, and then the migration runner would skip it if the current node had just performed a bootstrap. However, the respective migration would still be run (aimlessly) by a 2nd node joining the cluster, or even by the same node that just skipped them, after a restart. This patch improves the mechanism such that migrations included in the bootstrap image are never run if the cluster's bootstrap version is high enough. It does so by allowing one to mark migrations with the version at which they've been introduced, and the runner will skip them if the cluster's bootstrap version is >=. This means that migrations that want to be marked in this way also need to introduce a cluster version. The benefit of this change, besides faster node startup time, removal of confusing entries from the system.event_log table and more determinism (because now some migrations are never run instead of not run on single node clusters but run on multi-node clusters), is that logic tests that hardcode expected cluster events are more easily written. Before this patch, those tests had to explicitly ignore some events that are generated by migrations (because, again, depending on the configuration, the migration would sometimes run and sometimes not). It's now the second time this happens to a change that I personally want to make, and it's very surprising and scary when it happens to you. You can see the edit in the event_log logic test for a case that I'm not correcting (there's probably others in the same file but I haven't looked). Release note: None. 41991: storage/engine: move MVCCIncrementalIterator from storageccl to engine r=petermattis a=hueypark This is prerequisites for [storage/engine: add Pebble support to ExportToSst](#41739). Recovered some tests from [engine: Add ExportToSst method with all logic moved to C++](75f1314). See #41945 Release note: None 42066: sql: SHOW ZONE CONFIGURATION only if user has permissions r=otan a=otan Previously, SHOW ZONE CONFIGURATION did not respect permissions on tables. Patch that up by checking permissions when using this command. Resolves the command in #40917. Has merge conflicts with #42046; I will fix them up with whatever lands first. Release note (bug fix): fix bug where zones would display in `SHOW ZONE CONFIGURATION` even though the user has no permission to view that table Co-authored-by: Andrei Matei <andrei@cockroachlabs.com> Co-authored-by: Jaewan Park <jaewan.huey.park@gmail.com> Co-authored-by: Oliver Tan <otan@cockroachlabs.com>
Most of these commits are from engine: Add ExportToSst method with all logic moved to C++. It is not currently testable (PASS on my local) and can be checked when roachtest: add flag (or env var) to request pebble #4162 is completed.
Fixes #41739
Release note: None