Adar Dembo has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata ......................................................................
Patch Set 15: (5 comments) Came to +2, ended up leaving some comments. :/ http://gerrit.cloudera.org:8080/#/c/3823/15//COMMIT_MSG Commit Message: PS15, Line 15: once after Nit: seems like you only need one of these words, but not both. http://gerrit.cloudera.org:8080/#/c/3823/15/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: PS15, Line 271: fix(KUDU-1500) Nit: "fix (KUDU-1500)" (separate 'fix' from the parenthetical block with a space). PS15, Line 325: scoped_refptr<Thread> thread; : ASSERT_OK(kudu::Thread::Create(CURRENT_TEST_NAME(), : Substitute("list-tablet-thread-$0", i), : &ListTabletsDuringTabletCopy, : follower_ts, tablet_id, : &barrier, &finish, &thread)); : threads.push_back(thread); Now that we can use C++11 we prefer std::thread for test threads. The advantage is less boilerplate code, both in starting up threads and in the use of lambdas for the thread functions themselves, with lambda capture reducing parameter passing. See master-test.cc:TestConcurrentCreateOfSameTable for an example. http://gerrit.cloudera.org:8080/#/c/3823/15/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: PS15, Line 304: // We treat few fields in the metadata as immutable objects, : // so they are loaded from protobuf only when tablet replica : // is being instantiated on this peer(KUDU-1500). Nit: this comment is a little confusing. How about rewording like so: "Some metadata fields are assumed to be immutable and thus are only read from the protobuf when the tablet metadata is loaded for the very first time. See KUDU-1500 for more details." PS15, Line 313: DCHECK_EQ(table_id_, superblock.table_id()); : PartitionSchemaPB partition_schema_pb; : partition_schema_.ToPB(&partition_schema_pb); : DCHECK_EQ(superblock.partition_schema().SerializeAsString(), : partition_schema_pb.SerializeAsString()); : PartitionPB partition_pb; : partition_.ToPB(&partition_pb); : DCHECK_EQ(superblock.partition().SerializeAsString(), : partition_pb.SerializeAsString()); I have some questions about this logic: 1. Since this is just sanity checks that are enabled in debug builds, the entire block should be conditioned on #ifndef NDEBUG (and then the DCHECK_EQs should be converted into CHECK_EQ). This is to avoid the ToPB() calls in non-debug builds, where their results aren't used. 2. I'm worried about impacting forward compatibility by comparing serialized protobufs like this. In commit 43d2749, Mike removed an equivalent check in tserver registration which effectively prevented us from adding new fields to the registration protobuf. Would this lead to the same problem? Or is it different? -- To view, visit http://gerrit.cloudera.org:8080/3823 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019 Gerrit-PatchSet: 15 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
