storage/engine: move MVCCIncrementalIterator from storageccl to engine by hueypark · Pull Request #41991 · cockroachdb/cockroach

hueypark · 2019-10-29T15:15:08Z

This is prerequisites for storage/engine: add Pebble support to ExportToSst. Recovered some tests from engine: Add ExportToSst method with all logic moved to C++.

See #41945

Release note: None

cockroach-teamcity · 2019-10-29T15:15:18Z

This change is

petermattis

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @hueypark)

pkg/storage/engine/mvcc_incremental_iterator_test.go, line 95 at r1 (raw file):

}

func TestMVCCIterateIncremental(t *testing.T) {

Did these tests come from somewhere or are they brand new? I need to scrutinize them more. My first reaction is "quite nice", though for many of the MVCC layer tests I'd like to be pushing towards using the datadriven framework. That isn't necessary in this PR, I'm mostly just curious about their pedigree.

pkg/storage/engine/mvcc_incremental_iterator.go, line 53 at r1 (raw file):

// oracle to prove the correctness of the new export logic.
type MVCCIncrementalIterator struct {
	// TODO(dan): Move all this logic into c++ and make this a thin wrapper.

You can remove this TODO. There is a C++ version of all of this logic.s

hueypark

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @petermattis)

pkg/storage/engine/mvcc_incremental_iterator_test.go, line 95 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Did these tests come from somewhere or are they brand new? I need to scrutinize them more. My first reaction is "quite nice", though for many of the MVCC layer tests I'd like to be pushing towards using the datadriven framework. That isn't necessary in this PR, I'm mostly just curious about their pedigree.

It' deleted tests from engine: Add ExportToSst method with all logic moved to C++.

pkg/storage/engine/mvcc_incremental_iterator.go, line 53 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

You can remove this TODO. There is a C++ version of all of this logic.s

Done.

petermattis

modulo a few small comments.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @hueypark)

pkg/storage/engine/mvcc_incremental_iterator_test.go, line 95 at r1 (raw file):

Previously, hueypark (Jaewan Park) wrote…

It' deleted tests from engine: Add ExportToSst method with all logic moved to C++.

Ack.

Nit: s/TestMVCCIterateIncremental/MVCCIncrementalIterator/g for mildly improved grep'ability of this code.

pkg/storage/engine/mvcc_incremental_iterator_test.go, line 129 at r2 (raw file):

	kvs := func(kvs ...MVCCKeyValue) []MVCCKeyValue { return kvs }

	runWithAllEngines(func(e Engine, t *testing.T) {

I have a TODO to replace runWithAllEngines with:

	for _, engineImpl := range mvccEngineImpls {
		t.Run(engineImpl.name, func(t *testing.T) {
			engine := engineImpl.create()
			defer engine.Close()

This allows us to select the subtests from the command line. We could technically fix runWithAllEngines to use subtests, but I'd like to have one mechanism, and the for-loop is used more widely.

danhhz · 2019-10-30T14:56:04Z

I briefly looked into why this had to move out of ccl and it seems like it's because storage.ExportToSst wants to use it? ExportToSst is only used from ccl code, so I'm really not sure why it is in storage in the first place. We've promised not to move apache2 code to ccl so I guess we can't fix it at this point. Unfortunate

petermattis · 2019-10-30T15:02:03Z

@danhhz Note that the C++ version of incremental iterator was already in non-ccl code. That might have been an error when that code was written, but I don't think this changes the situation materially.

danhhz · 2019-10-30T15:17:30Z

Yeah, I don't see any reason that the C++ version of incremental iterator should be outside of ccl code either. I wonder if there's anything easy we can do to have better discipline about this going forward.

MVCCIncrementalIterator is a decent piece of complexity at the moment and it really is unfortunate to move it out of ccl. What do you think about leaving it where it is but adding a hook (similar to the other non-ccl -> ccl hooks) so that ExportToSst can use it? I suspect it probably wouldn't work well since ExportToSst would have to use it in terms of some interface

petermattis · 2019-10-30T15:29:10Z

It seems weird to jump through hoops to keep the Go version CCL while the C++ version is already non-CCL.

danhhz · 2019-10-30T15:45:13Z

But isn't the c++ version going away when rocksdb does? If so, it doesn't seem to me to be a good precedent.

Anyway, it seems like you're okay with this. I'm a little concerned about the meta of how easily this cascaded, but ultimately it's your call

petermattis · 2019-10-30T16:00:39Z

@danhhz Do you want ExportToSst to exist in CCL code? Currently it exists in non-CCL C++ code.

danhhz · 2019-10-30T18:07:33Z

I would have put ExportToSst and DBExportToSst in ccl, but at this point our hands are tied, no?

My bigger worry is that it ended up out of ccl in the first place and that's now having a cascading effect. Are we worried that's going to happen again?

petermattis · 2019-10-30T20:50:57Z

My bigger worry is that it ended up out of ccl in the first place and that's now having a cascading effect. Are we worried that's going to happen again?

Yeah. Do you know what happened there? I don't.

danhhz · 2019-10-30T20:58:50Z

Looks like it happened in #37496. I peeked and as of this PR, MVCCIncrementalIterator was still in ccl (pkg/ccl/storageccl/engineccl/mvcc.go), so not sure why the c++ port of it ended up outside ccl. The right people (david) were on the review. I don't see any discussion in the review about ccl.

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

hueypark

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @petermattis)

pkg/storage/engine/mvcc_incremental_iterator_test.go, line 95 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

Ack.

Nit: s/TestMVCCIterateIncremental/MVCCIncrementalIterator/g for mildly improved grep'ability of this code.

Done.

pkg/storage/engine/mvcc_incremental_iterator_test.go, line 129 at r2 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I have a TODO to replace runWithAllEngines with:
	for _, engineImpl := range mvccEngineImpls {
		t.Run(engineImpl.name, func(t *testing.T) {
			engine := engineImpl.create()
			defer engine.Close()
This allows us to select the subtests from the command line. We could technically fix runWithAllEngines to use subtests, but I'd like to have one mechanism, and the for-loop is used more widely.

I created an issue for this: #42060

petermattis

@hueypark the change here looks good, but we're still figuring out code structuring.

@danhhz So what should we do here? Should we keep the Go version of incremental iterator in CCL code?

Cc @dt in case he has opinions.

Reviewable status: complete! 0 of 0 LGTMs obtained (and 1 stale)

danhhz · 2019-10-31T14:42:24Z

@danhhz So what should we do here? Should we keep the Go version of incremental iterator in CCL code?

Do you see a good way to keep the go incremental iterator in ccl? I do not. To use the hook method, we'd have to create some interface in non-ccl code that it implements and write ExportToSst in terms of it. I doubt the performance here would be good. I see 3 options:

Move go incremental iterator out of ccl. A lot of export/import has been moving out of ccl anyway (see ExportStorage), so maybe we've lost this battle. Learn our lesson and figure out if there's something additional we should be doing to prevent this from happening to changefeeds, etc that are still firmly in ccl.
ExportToSst was added in the 19.2 cycle. We've previously been okay with moving stuff into ccl when it was merged apache2 by mistake as long as it hasn't made it into a major release. So, sneak a PR that moves ExportToSst to ccl into 19.2.0.
Same as 2 but 19.2.1.

I'm happy with 1 or 3. Thoughts? Is there an option I'm missing?

Cc @dt in case he has opinions.

He's OOO, but calendar indicates he's back tomorrow? If possible, I would like his opinion here before we commit to anything.

danhhz · 2019-10-31T16:03:48Z

@petermattis David pinged me from his phone and mentioned that there was discussion about this when it merged (which makes sense, I would have been quite surprised if something like this had slipped by david in code review). There was something about cgo not liking to use a symbol (C.DBEngine) cross-packages that prevented ExportToSst from being in ccl. I searched around for the thread he mentioned but couldn't find it.

This reasoning probably should be been a comment on ExportToSst to head off this sort of later confusion, but beyond that it doesn't seem there's anything to do here. I withdraw my objection.

petermattis · 2019-10-31T18:39:19Z

Hmm, I think there is something that could have been done regarding the cgo issue. Thanks for tracking down what happened.

@hueypark Thanks for the contribution!

bors r+

craig · 2019-10-31T19:06:43Z

Build failed

GitHub CI (Cockroach)

petermattis · 2019-10-31T20:41:07Z

Unrelated gossip/restart failure. But one that deserves to be investigated. I'll file an issue if one doesn't already exist.

bors r+

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>

craig · 2019-10-31T21:30:33Z

Build succeeded

GitHub CI (Cockroach)

petermattis approved these changes Oct 30, 2019

View reviewed changes

hueypark force-pushed the move-iterator branch from d9ef0d8 to ba4db5e Compare October 30, 2019 13:10

hueypark commented Oct 30, 2019

View reviewed changes

petermattis approved these changes Oct 30, 2019

View reviewed changes

hueypark mentioned this pull request Oct 31, 2019

storage/engine: replace multiple engine testing methods from runWithAllEngines to for _, engineImpl := range mvccEngineImpls #42060

Closed

hueypark force-pushed the move-iterator branch from ba4db5e to 7cfcaff Compare October 31, 2019 12:11

hueypark commented Oct 31, 2019

View reviewed changes

petermattis reviewed Oct 31, 2019

View reviewed changes

craig bot merged commit 7cfcaff into cockroachdb:master Oct 31, 2019

Conversation

hueypark commented Oct 29, 2019

Uh oh!

cockroach-teamcity commented Oct 29, 2019

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

hueypark left a comment

Choose a reason for hiding this comment

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

danhhz commented Oct 30, 2019

Uh oh!

petermattis commented Oct 30, 2019

Uh oh!

danhhz commented Oct 30, 2019

Uh oh!

petermattis commented Oct 30, 2019

Uh oh!

danhhz commented Oct 30, 2019

Uh oh!

petermattis commented Oct 30, 2019

Uh oh!

danhhz commented Oct 30, 2019

Uh oh!

petermattis commented Oct 30, 2019

Uh oh!

danhhz commented Oct 30, 2019

Uh oh!

hueypark left a comment

Choose a reason for hiding this comment

Uh oh!

petermattis left a comment

Choose a reason for hiding this comment

Uh oh!

danhhz commented Oct 31, 2019

Uh oh!

danhhz commented Oct 31, 2019

Uh oh!

petermattis commented Oct 31, 2019

Uh oh!

craig bot commented Oct 31, 2019

Build failed

Uh oh!

petermattis commented Oct 31, 2019

Uh oh!

craig bot commented Oct 31, 2019

Build succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants