Yingchun Lai 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 2: (22 comments) http://gerrit.cloudera.org:8080/#/c/19327/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/19327/2//COMMIT_MSG@9 PS2, Line 9: metadata nit: log container metadata http://gerrit.cloudera.org:8080/#/c/19327/2//COMMIT_MSG@10 PS2, Line 10: in nit: at http://gerrit.cloudera.org:8080/#/c/19327/2//COMMIT_MSG@12 PS2, Line 12: whick nit: which http://gerrit.cloudera.org:8080/#/c/19327/2//COMMIT_MSG@23 PS2, Line 23: metdata metadata http://gerrit.cloudera.org:8080/#/c/19327/2//COMMIT_MSG@24 PS2, Line 24: operations How about describe the influence of each operation? and the operation steps. 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* kContainerCompactedFileSuffix; : static const char* kContainerOffsetFileSuffix; : static const char* kContainerBackupFileSuffix; how about add 'Metadata' to the variable names? http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h@222 PS2, Line 222: static Status GetContainerNames(Env* env, const std::string& data_path, Add some comments to describe what the functions do. http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h@225 PS2, Line 225: Low nit: LowLiveRatio http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.h@228 PS2, Line 228: Low nit: LowLiveRatio 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 and find out the max offset, right? http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc@2582 PS2, Line 2582: Substitute Use JoinPathSegments, other places are the same. http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc@2586 PS2, Line 2586: env, : metadata_path, : &live_block_records, : &total_blocks, : &offset Make them in one line? Ident 4 spaces is enough, other places are the same. http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/fs/log_block_manager.cc@2599 PS2, Line 2599: vector<BlockRecordPB> records = LogBlockContainer::SortRecords( : std::move(live_block_records)); vector<BlockRecordPB> records = LogBlockContainer::SortRecords(std::move(live_block_records)); 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: value is nit: values are http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@918 PS2, Line 918: num nit: 'count' would be better. 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'? http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@940 PS2, Line 940: num nit: 'count' would be better. http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@945 PS2, Line 945: LOG how about log the live block ratio? http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@962 PS2, Line 962: LOG how about log the live block ratio too? http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@967 PS2, Line 967: LOG(INFO) << "Operation finished, some tasks failed."; It would be better to return a non-ok status, it would be easier to use for thirdparty tools. http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@1097 PS2, Line 1097: .AddOptionalParameter("metadata_op_type") nit: simplified by op_typ? since 'metadata' has been mentioned in action name. http://gerrit.cloudera.org:8080/#/c/19327/2/src/kudu/tools/tool_action_fs.cc@1108 PS2, Line 1108: .AddAction(std::move(compact_metadata)) keep the actions in lexicographical order. -- 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: 2 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: Sat, 17 Dec 2022 07:16:52 +0000 Gerrit-HasComments: Yes
