Adar Dembo has posted comments on this change.

Change subject: KUDU-1500: Data race in 

Patch Set 15:


Came to +2, ended up leaving some comments. :/
Commit Message:

PS15, Line 15: once after
Nit: seems like you only need one of these words, but not both.
File src/kudu/integration-tests/

PS15, Line 271: fix(KUDU-1500)
Nit: "fix (KUDU-1500)" (separate 'fix' from the parenthetical block with a 

PS15, Line 325:     scoped_refptr<Thread> thread;
              :     ASSERT_OK(kudu::Thread::Create(CURRENT_TEST_NAME(),
Substitute("list-tablet-thread-$0", i),
              :                                    &ListTabletsDuringTabletCopy,
              :                                    follower_ts, tablet_id,
              :                                    &barrier, &finish, &thread));
              :     threads.push_back(thread);
Now that we can use C++11 we prefer std::thread for test threads. The advantage 
is less boilerplate code, both in starting up threads and in the use of lambdas 
for the thread functions themselves, with lambda capture reducing parameter 

See for an example.
File src/kudu/tablet/

PS15, Line 304:     // We treat few fields in the metadata as immutable objects,
              :     // so they are loaded from protobuf only when tablet replica
              :     // is being instantiated on this peer(KUDU-1500).
Nit: this comment is a little confusing. How about rewording like so: "Some 
metadata fields are assumed to be immutable and thus are only read from the 
protobuf when the tablet metadata is loaded for the very first time. See 
KUDU-1500 for more details."

PS15, Line 313:       DCHECK_EQ(table_id_, superblock.table_id());
              :       PartitionSchemaPB partition_schema_pb;
              :       partition_schema_.ToPB(&partition_schema_pb);
              :                 partition_schema_pb.SerializeAsString());
              :       PartitionPB partition_pb;
              :       partition_.ToPB(&partition_pb);
              :       DCHECK_EQ(superblock.partition().SerializeAsString(),
              :                 partition_pb.SerializeAsString());
I have some questions about this logic:
1. Since this is just sanity checks that are enabled in debug builds, the 
entire block should be conditioned on #ifndef NDEBUG (and then the DCHECK_EQs 
should be converted into CHECK_EQ). This is to avoid the ToPB() calls in 
non-debug builds, where their results aren't used.
2. I'm worried about impacting forward compatibility by comparing serialized 
protobufs like this. In commit 43d2749, Mike removed an equivalent check in 
tserver registration which effectively prevented us from adding new fields to 
the registration protobuf. Would this lead to the same problem? Or is it 

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: If57617e22b41296b8d4e8ad131220f1ebb235019
Gerrit-PatchSet: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Dinesh Bhat <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <>
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to