Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1500: Data race in 

Patch Set 10:


TFTR Mike, updated new patch, and also responses inline below.

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.

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.
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.

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_;

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

Reply via email to