Mike Percy has posted comments on this change.

Change subject: KUDU-1500: Data race in 

Patch Set 8:


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.

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

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 

Line 327:       << " on TS " << follower_ts->uuid();
nit: it's prettier / more readable to align the << with the one on the previous 

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());

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 

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