Yingchun Lai 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: (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 Done 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 expe Reserved in L2735, but it make sense to move it here. Done. 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 th Done 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 m Done 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? Done 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 onl Done 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? Good feature since C++ 17 ! Done 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. Done 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? Done 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 Done 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? Done http://gerrit.cloudera.org:8080/#/c/19519/11/src/kudu/fs/log_block_manager.cc@2763 PS11, Line 2763: , > ditto Done 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 Done -- 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 15:44:35 +0000 Gerrit-HasComments: Yes
