Skip to content

engine: fix different serialization of MVCCMetadata used for#45973

Merged
craig[bot] merged 1 commit intocockroachdb:masterfrom
sumeerbhola:merge_marshal
Mar 11, 2020
Merged

engine: fix different serialization of MVCCMetadata used for#45973
craig[bot] merged 1 commit intocockroachdb:masterfrom
sumeerbhola:merge_marshal

Conversation

@sumeerbhola
Copy link
Copy Markdown
Collaborator

non-MVCC data that is merged using C++ code in RocksDB and
Go code in Pebble

The serialization now matches the C++ code.

Fixes #45811

Release note: None

@cockroach-teamcity
Copy link
Copy Markdown
Member

This change is Reviewable

Copy link
Copy Markdown
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, though I've got one more test for you to enhance/add.

PS I appreciate the use of a very unattractive name for this proto.

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


pkg/storage/enginepb/mvcc.proto, line 89 at r1 (raw file):

  option (gogoproto.goproto_stringer) = false;
  option (gogoproto.equal) = true;
  option (gogoproto.populate) = true;

I believe this is only used by kv/kvserver/below_raft_protos_test.go. It would be nice to verify that the serialization of this proto doesn't accidentally changed, because the serialization happens below Raft. But I'm not sure if you can hook directly into the below_raft_protos_test.go due to the way those golden files are checked that they are still being marshaled below Raft. If you can hook into that test, great. If you can't, I'd suggest forking the relevant portions (i.e. randomly populating a proto using a fixed seed and verifying the output when serialized).

Copy link
Copy Markdown
Collaborator Author

@sumeerbhola sumeerbhola left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TFTR!
Modified below_raft_protos_test.go

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


pkg/storage/sst_writer_test.go, line 148 at r2 (raw file):

	require.Equal(t, string(sstRocks), string(sstPebble))
	itRocks.Close()
	itPebble.Close()

this is unrelated -- the test was failing with a pebble: cache has non-zero reference count error. Not sure why it has not been consistently failing.


pkg/storage/enginepb/mvcc.proto, line 89 at r1 (raw file):

Previously, petermattis (Peter Mattis) wrote…

I believe this is only used by kv/kvserver/below_raft_protos_test.go. It would be nice to verify that the serialization of this proto doesn't accidentally changed, because the serialization happens below Raft. But I'm not sure if you can hook directly into the below_raft_protos_test.go due to the way those golden files are checked that they are still being marshaled below Raft. If you can hook into that test, great. If you can't, I'd suggest forking the relevant portions (i.e. randomly populating a proto using a fixed seed and verifying the output when serialized).

You were right -- running the kvserver tests with COCKROACH_STORAGE_ENGINE=pebble fails with
*enginepb.MVCCMetadataSubsetForMergeSerialization: missing fixture! Please adjust belowRaftGoldenProtos if necessary

Copy link
Copy Markdown
Collaborator

@petermattis petermattis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

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


pkg/storage/sst_writer_test.go, line 148 at r2 (raw file):

Previously, sumeerbhola wrote…

this is unrelated -- the test was failing with a pebble: cache has non-zero reference count error. Not sure why it has not been consistently failing.

GC needs to fire in order for the finalizer-based leak detection to trigger. I wouldn't be surprised if there are one or two more such places lurking. It has been straightforward to track down the missing Close calls in my experience.

non-MVCC data that is merged using C++ code in RocksDB and
Go code in Pebble

The serialization now matches the C++ code.

Fixes cockroachdb#45811

Release note: None

Release justification: Fixes replica divergence when running cluster
with both Pebble and RocksDB
@sumeerbhola
Copy link
Copy Markdown
Collaborator Author

bors r+

@craig
Copy link
Copy Markdown
Contributor

craig bot commented Mar 11, 2020

Build succeeded

@craig craig bot merged commit e70c4ee into cockroachdb:master Mar 11, 2020
@sumeerbhola sumeerbhola deleted the merge_marshal branch March 11, 2020 19:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

roachtest: engine/switch/nodes=3 failed

3 participants