Dinesh Bhat has posted comments on this change.

Change subject: KUDU-1500: Data race in 

Patch Set 15:


TFTR Adar, I will update the new patch in a while, please see responses inline.

File src/kudu/integration-tests/tablet_copy-itest.cc:

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 advan
TBH I need to read up about std::thread to understand what you are saying about 
these subtle differences. I will post an update once I convert this into 

File src/kudu/tablet/tablet_metadata.cc:

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

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. I actually thought about NDEBUG, but forgot to materialize in between the 
test experiments, good that you reminded :), consider it done.

2. Hmmm, good Qn. I believe we may land into similar issue if we change the 
superblock version(or format). i.e, destination version differs from source in 
the tablet-copy workflow. My hunch is that(not very sure) we probably have to 
worry about that compatibility at several other places more than here. However 
Mike may have more clearer picture here to answer this Qn.

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: 15
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@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