Dinesh Bhat has posted comments on this change. Change subject: KUDU-1500: Data race in RaftConsensusITest.TestCorruptReplicaMetadata ......................................................................
Patch Set 10: (10 comments) TFTR Mike, updated new patch, and also responses inline below. 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. > 4 spaces is the minimum. ok cool, i may have missed 'minimum' part. 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 Fixed. Line 254: Status ListTabletsDuringTabletCopy(const TServerDetails* ts, > Mind adding a quick doc comment to this function? Done. Line 276: // This is only optimistic in that, we hope to catch the > Worth mentioning that we rely on TSAN to "catch" it. added some more verbiage, the line above had a tsan mention though. Line 322: std::atomic<bool> transition(false); > I don't think we use the transition variable anymore. good catch, removed. Line 324: int num_threads = FLAGS_test_num_threads; > why bother having this extra variable num_threads? good point, removed. that also makes the actual ListTablet thread lot thinner :) Line 355: finish = true; > It's only a test, but we can get away with finish.store(true, memory_order_ my thinking was since this is a test we could get away with the memory barrier :), but I see your point and agree with you, done. 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(); > Are you sure table_name_ is not accessed by ListTablets()? I was speaking from immutability point of view. To answer your Qn, table_name_ is accessed all over including ListTablets, but in safe way. The same lock which is held during SuperblockLoading is held across reads for table_name_ too. string TabletMetadata::table_name() const { std::lock_guard<LockType> l(data_lock_); DCHECK_NE(state_, kNotLoadedYet); return table_name_; } http://gerrit.cloudera.org:8080/#/c/3823/10/src/kudu/tablet/tablet_metadata.cc File src/kudu/tablet/tablet_metadata.cc: Line 317: DCHECK_EQ( > Although, another thing to consider is whether there is always a guaranteed Hmmmm, good point. Tests on localhost passing, I will run various tests on different platforms exercising this routine to make sure assumption holds true. Line 323: DCHECK_EQ(0, table_id_.compare(superblock.table_id())); > DCHECK_EQ(table_id_, superblock.table_id()) Ha ! didn't realize DCHECK_CHECK actually acts on string. thank you. Took care of all the above comments. -- 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
