Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1500: Data race in 

Patch Set 8:


TFTR MIke, addressed all the comments, and also re-ran the test suite. Please 
take a look at responses inline.

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

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

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.

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

Line 303:     if (state_ == kNotLoadedYet) {
> Please add a comment here explaining why we only do this when the TabletMet

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