Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/21779 )
Change subject: [tablet] fix race in RowSetMetadata::id() ...................................................................... Patch Set 3: (2 comments) http://gerrit.cloudera.org:8080/#/c/21779/3/src/kudu/tablet/cfile_set.cc File src/kudu/tablet/cfile_set.cc: http://gerrit.cloudera.org:8080/#/c/21779/3/src/kudu/tablet/cfile_set.cc@148 PS3, Line 148: const auto& ins_info = readers_by_col_id_.emplace(col_id, std::move(reader)); : DCHECK(ins_info.second) << "already open"; > What about using EmplaceOrDie to simplify the code? The original code has it as DCHECK, and I'm not sure we want to expand this to release builds as well, so I opted using this approach instead of blanket EmplaceOrDie. http://gerrit.cloudera.org:8080/#/c/21779/3/src/kudu/tablet/rowset_metadata.cc File src/kudu/tablet/rowset_metadata.cc: http://gerrit.cloudera.org:8080/#/c/21779/3/src/kudu/tablet/rowset_metadata.cc@127 PS3, Line 127: void RowSetMetadata::ToProtobuf(RowSetDataPB *pb) { > We can set ToProtobuf() as const by the way? Good point! I'd rather do so in a separate follow-up patch along with other minor improvements on the code: the idea is to keep this patch focused on addressing the race condition. -- To view, visit http://gerrit.cloudera.org:8080/21779 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I4b09575616e754b7dbb24586293f128e361b9360 Gerrit-Change-Number: 21779 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Mahesh Reddy <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 17 Sep 2024 19:03:03 +0000 Gerrit-HasComments: Yes
