Mike Percy has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata ......................................................................
Patch Set 8: (13 comments) http://gerrit.cloudera.org:8080/#/c/3823/8/src/kudu/integration-tests/cluster_itest_util.h File src/kudu/integration-tests/cluster_itest_util.h: Line 242: Status ListTabletsDuringTabletCopy(const TServerDetails* ts, I don't think this utility is generic enough to put here. It is very specific. Let's move it to the test. 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 42: DEFINE_int32(num_test_threads, 16, Let's follow the convention of the other params and name this test_num_threads Line 262: // Step 4: Verify that at least one thread caught the state transition. Let's just remove step 4. Line 287: for (int i = 0; i < cluster_->num_tablet_servers(); i++) { This check is not needed. WaitForServersToAgree() already effectively achieves this. Line 298: // Find out who is leader so that we can tombstone a follower It doesn't matter whether you kill a leader or a follower, does it? We only care that one of the servers eventually becomes leader and starts a tablet copy on our tombstoned replica. That said, even though this isn't necessary for correctness, it makes average test time lower since we don't have to wait for an election to get the tablet copy kicked off. Let's just document that as the reason for doing this. Line 306: AtomicBool transition(false); These days, prefer std::atomic_bool or std::atomic<bool> (the former is a typedef of the latter) from C++11 to this older C++03 compatible atomics library. Line 327: << " on TS " << follower_ts->uuid(); nit: it's prettier / more readable to align the << with the one on the previous line Line 347: if (!transition.Load()) Hmm. I think we should remove this since it's not really doing anything, and trying to rely on it would make this test racy. No one will ever check this test log output and if they did it wouldn't tell them very much. Line 351: ASSERT_TRUE(cluster_->tablet_server(kTsIndex)->IsProcessAlive()); This can be replaced with NO_FATALS(cluster_->AssertNoCrashes()); http://gerrit.cloudera.org:8080/#/c/3823/8/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 283: table_id_ = superblock.table_id(); What about table_id_? Shouldn't this also be inside the "if" statement below? Also the table_name_. Right now, we don't support renaming tables. I think we should add a DHECK that these things didn't change and then put them inside the "if". Line 298: LOG_WITH_PREFIX(WARNING) << "Upgrading from Kudu 0.5.0 directly to this" This seems like it should be a FATAL error to me. Dan? Line 300: << " moving to a higher version." << std::endl; std::endl is not required here and should be removed Line 303: if (state_ == kNotLoadedYet) { Please add a comment here explaining why we only do this when the TabletMetadata is uninitialized. -- 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: 8 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Dinesh Bhat <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
