Wang Xixu has posted comments on this change. ( http://gerrit.cloudera.org:8080/19327 )
Change subject: [Tool] Compact metadata file for old versions of Kudu ...................................................................... Patch Set 3: (17 comments) > Patch Set 2: > > (22 comments) http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h File src/kudu/fs/log_block_manager.h: http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h@187 PS2, Line 187: static const char* kMetadataCompactedFileSuffix; : static const char* kMetadataOffsetFileSuffix; : static const char* kMetadataBackupFileSuffix; > how about add 'Metadata' to the variable names? Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h@222 PS2, Line 222: // Get all container name of one data_dir. > Add some comments to describe what the functions do. Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h@225 PS2, Line 225: > nit: LowLiveRatio Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h@228 PS2, Line 228: > nit: LowLiveRatio Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc File src/kudu/fs/log_block_manager.cc: http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc@867 PS2, Line 867: read_status = pb_reader.ReadNextPB(&record); > I doubt if we can skip parsing the message, we only need to iterate file an No. It is need to parse the record and find live blocks. http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc@2582 PS2, Line 2582: StrCat(Joi > Use JoinPathSegments, other places are the same. Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc@2586 PS2, Line 2586: ecords.size(); : total_blocks = total_blocks == 0 ? 1 : total_blocks; : if (static_cast<double>(live_block_records.size()) / : total_blocks <= FLAGS_log_container_live_metadata_before_compact_ratio) { : LOG(INFO) << "Compact low block con > Make them in one line? Ident 4 spaces is enough, other places are the same. Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc@2599 PS2, Line 2599: RETURN_NOT_OK(GenerateOffsetFile(env, dir, container_name, offset)); : } > vector<BlockRecordPB> records = Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@123 PS2, Line 123: values a > nit: values are Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@918 PS2, Line 918: cou > nit: 'count' would be better. Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@937 PS2, Line 937: , > Why not log result status for 'clear'? Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@940 PS2, Line 940: int > nit: 'count' would be better. Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@945 PS2, Line 945: > how about log the live block ratio? Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@962 PS2, Line 962: ol- > how about log the live block ratio too? Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@967 PS2, Line 967: << ", live block ratio: " << (total_live_blocks / total_blocks); > It would be better to return a non-ok status, it would be easier to use for Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@1097 PS2, Line 1097: .AddOptionalParameter("fs_metadata_dir") > nit: simplified by op_typ? since 'metadata' has been mentioned in action na Done http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@1108 PS2, Line 1108: .AddAction(std::move(format)) > keep the actions in lexicographical order. Done -- To view, visit http://gerrit.cloudera.org:8080/19327 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id61cb6c88d7a093a8448e2497639f5c5c094efa4 Gerrit-Change-Number: 19327 Gerrit-PatchSet: 3 Gerrit-Owner: Wang Xixu <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Wang Xixu <[email protected]> Gerrit-Reviewer: Yingchun Lai <[email protected]> Gerrit-Comment-Date: Mon, 19 Dec 2022 02:00:03 +0000 Gerrit-HasComments: Yes
