Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/19519 )
Change subject: [LBM] append CREATE/DELETE records in batch to metadata ...................................................................... Patch Set 11: Code-Review+1 (13 comments) http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@491 PS11, Line 491: the block ids deleted successfully are returned by 'deleted_block_ids'. It would be great to point to the fact this holds true even if the function returns non-OK status. http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@1411 PS11, Line 1411: deleted_block_ids Does it make sense to reserve the space in 'deleted_block_ids' for the expected number of elements? http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@1418 PS11, Line 1418: shared_lock<RWMutex> l(metadata_compact_lock_); If exploring batch-like approach, why not to take advantage of acquiring this lock only once when appending multiple records to metadata_file_? Is it possible to move this lock out of this for() cycle? Alternatively, it's possible to create a temporary vector populated with BlockIdPB or even with BlockRecordPB, and then acquire the metadata_compact_lock_ once, and iterate over the temporary vector, calling deleted_block_ids_->Append() for each element of the vector? http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@1427 PS11, Line 1427: // Note: We don't check for sufficient disk space for metadata writes in : // order to allow for block deletion on full disks. How is this relevant to AppendMetadataForCreateRecords()? Should this be moved into AppendMetadataForDeleteRecords() instead? http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@1432 PS11, Line 1432: const auto& nit: should this be 'const auto*' because the elements are pointers? http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@1436 PS11, Line 1436: shared_lock<RWMutex> l(metadata_compact_lock_); Ditto: is it possible to move this lock out of the cycle and acquire it only once? http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2723 PS11, Line 2723: lbs_by_container nit: is it possible to use structured binding? for (auto& [container, clbs] : lbs_by_containers) { ... } http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2749 PS11, Line 2749: Purge the log blocks which are deleted failed. nit: Purge the blocks that failed to delete. http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2761 PS11, Line 2761: deleted->insert(deleted->end(), deleted_block_ids.begin(), deleted_block_ids.end()); nit: could use std::move() here? std::move(deleted_block_ids.begin(), deleted_block_ids.end(), std::back_inserter(*deleted)); http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2763 PS11, Line 2763: , nit: add space after the comma http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2763 PS11, Line 2763: , ditto http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2763 PS11, Line 2763: log_blocks->insert nit: could this be rewritten using std::move() as well? http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2777 PS11, Line 2777: Can't find block when remove log block nit: how about cannot find block to remove -- To view, visit http://gerrit.cloudera.org:8080/19519 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie24adab3b1dbbea55108a1f525093136fcf5a726 Gerrit-Change-Number: 19519 Gerrit-PatchSet: 11 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: KeDeng <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yifan Zhang <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Reviewer: Yuqi Du <[email protected]> Gerrit-Comment-Date: Wed, 08 Mar 2023 07:26:22 +0000 Gerrit-HasComments: Yes
