Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/17871 )
Change subject: KUDU-3318 [LBM] Runtime compact log container metadata ...................................................................... Patch Set 9: Code-Review+1 (4 comments) http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@202 PS9, Line 202: F nit: align this with the "(" http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@495 PS9, Line 495: enum class ProcessRecordType { : kReadOnly, // Read records only. : kReadAndUpdate, // Read records and update container's statistic. : }; nit: can you add a comment mentioning why this is used, so users can better understand when to use it? E.g. "Whether to update internal counters based on processed records. This may be useful to avoid recomputing container statistics during operations that don't change them, e.g. compacting container metadata." http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@629 PS9, Line 629: if (static_cast<double>(live_blocks()) / total_blocks() >= : FLAGS_log_container_live_metadata_before_compact_ratio) { : return false; : } Considering rewriting using multiplication to avoid the costlier division https://stackoverflow.com/questions/17883240/is-multiplication-faster-than-float-division http://gerrit.cloudera.org:8080/#/c/17871/9/src/kudu/fs/log_block_manager.cc@2647 PS9, Line 2647: std::lock_guard<simple_spinlock> l(lock_); Now that 'all_containers_by_name_' isn't referenced here, we don't need this lock -- To view, visit http://gerrit.cloudera.org:8080/17871 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I72bdf73fa73403918c01dfa9d45530fad47ecaf6 Gerrit-Change-Number: 17871 Gerrit-PatchSet: 9 Gerrit-Owner: Yingchun Lai <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Tue, 26 Oct 2021 00:36:05 +0000 Gerrit-HasComments: Yes
