Dinesh Bhat has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata ......................................................................
Patch Set 8: (12 comments) TFTR MIke, addressed all the comments, and also re-ran the test suite. Please take a look at responses inline. 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_thre Done. Line 262: // Step 4: Verify that at least one thread caught the state transition. > Let's just remove step 4. Done. Line 287: for (int i = 0; i < cluster_->num_tablet_servers(); i++) { > This check is not needed. WaitForServersToAgree() already effectively achie Yeah, bad copy paste skills :), done. 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 Sounds good, that's true - that was the original intention of choosing a follower to tombstone to keep this test specifically geared towards kudu-1500. Line 306: AtomicBool transition(false); > These days, prefer std::atomic_bool or std::atomic<bool> (the former is a t Done. Line 327: << " on TS " << follower_ts->uuid(); > nit: it's prettier / more readable to align the << with the one on the prev thought of following 'function call' formatting guideline. Shouldn't next line be offset by 4 spaces ? Took your suggestion anyways. Line 347: if (!transition.Load()) > Hmm. I think we should remove this since it's not really doing anything, an Done. Agreed. Line 351: ASSERT_TRUE(cluster_->tablet_server(kTsIndex)->IsProcessAlive()); > This can be replaced with NO_FATALS(cluster_->AssertNoCrashes()); Done, thanks. 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 belo table_name_ is being set by AlterSchema, so even though it's not supported, we could keep it mutable(as-is) as long as it is not racy. I liked idea of doing DCHECK on all other 3 objects inside an else{} that they didn't differ from our assumption, added code and also tested them. Please take a look again. I am also getting this warning for the memcmp usage: "/Users/dinesh/Documents/kudu_exp/kudu/src/kudu/tablet/tablet_metadata.cc:317:140: warning: first operand of this 'memcmp' call is a pointer to dynamic class 'PartitionPB'; vtable pointer will be compared [-Wdynamic-class-memaccess]" 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? Hmmm, I kept it as warning for now, but I guess I was supposed to return error from here, just fixed that now. Line 300: << " moving to a higher version." << std::endl; > std::endl is not required here and should be removed Done. Line 303: if (state_ == kNotLoadedYet) { > Please add a comment here explaining why we only do this when the TabletMet Done. -- 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 <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
