Dinesh Bhat has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata ......................................................................
Patch Set 15: (4 comments) TFTR Adar, I will update the new patch in a while, please see responses inline. 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 Done 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 advan TBH I need to read up about std::thread to understand what you are saying about these subtle differences. I will post an update once I convert this into std::thread. 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 Done. 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. I actually thought about NDEBUG, but forgot to materialize in between the test experiments, good that you reminded :), consider it done. 2. Hmmm, good Qn. I believe we may land into similar issue if we change the superblock version(or format). i.e, destination version differs from source in the tablet-copy workflow. My hunch is that(not very sure) we probably have to worry about that compatibility at several other places more than here. However Mike may have more clearer picture here to answer this Qn. -- 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
