Mike Percy has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata ......................................................................
Patch Set 10: (13 comments) http://gerrit.cloudera.org:8080/#/c/3823/8/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: Line 327: // Threads to issue ListTablets RPCs to follower. > thought of following 'function call' formatting guideline. Shouldn't next l 4 spaces is the minimum. http://gerrit.cloudera.org:8080/#/c/3823/10/src/kudu/integration-tests/tablet_copy-itest.cc File src/kudu/integration-tests/tablet_copy-itest.cc: Line 23: #include <atomic> nit: alphabetical order for includes would put this at the top Line 254: Status ListTabletsDuringTabletCopy(const TServerDetails* ts, Mind adding a quick doc comment to this function? Line 276: // This is only optimistic in that, we hope to catch the Worth mentioning that we rely on TSAN to "catch" it. Line 322: std::atomic<bool> transition(false); I don't think we use the transition variable anymore. Line 324: int num_threads = FLAGS_test_num_threads; why bother having this extra variable num_threads? Line 355: finish = true; It's only a test, but we can get away with finish.store(true, memory_order_relaxed) here since we don't need a memory barrier. http://gerrit.cloudera.org:8080/#/c/3823/8/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 283: last_durable_mrs_id_ = superblock.last_durable_mrs_id(); > table_name_ is being set by AlterSchema, so even though it's not supported, Are you sure table_name_ is not accessed by ListTablets()? http://gerrit.cloudera.org:8080/#/c/3823/10/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 312: } else { Can you do the validation in the else-block in the same order that you do assignment in the if-block? Line 313: PartitionPB partitionPB; nit: camelCaps not allowed for variable names in C++ by the style guide. Also below. Name this partition_pb and the below one partition_schema_pb (also note there is a typo in the variable name "partion_schemaPB"). Line 317: DCHECK_EQ( I don't think this is valid. The easiest thing to do is DCHECK_EQ(superblock.partition().SerializeAsString(), partition_pb.SerializeAsString()); Line 320: DCHECK_EQ( Also here Line 323: DCHECK_EQ(0, table_id_.compare(superblock.table_id())); DCHECK_EQ(table_id_, superblock.table_id()) -- 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: 10 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@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
