engine: fix different serialization of MVCCMetadata used for#45973
engine: fix different serialization of MVCCMetadata used for#45973craig[bot] merged 1 commit intocockroachdb:masterfrom
Conversation
petermattis
left a comment
There was a problem hiding this comment.
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:
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).
c4f6f1a to
a362994
Compare
sumeerbhola
left a comment
There was a problem hiding this comment.
TFTR!
Modified below_raft_protos_test.go
Reviewable status:
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 thebelow_raft_protos_test.godue 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
petermattis
left a comment
There was a problem hiding this comment.
Reviewable status:
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 counterror. 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
a362994 to
fcc12aa
Compare
|
bors r+ |
Build succeeded |
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