Adar Dembo has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP) ......................................................................
Patch Set 1: (7 comments) Just did a quick first pass. http://gerrit.cloudera.org:8080/#/c/4305/1/docs/release_notes.adoc File docs/release_notes.adoc: Line 58: - The `kudu-fs_list` tool has been removed as it was replicating most of the As I mentioned in HipChat the other day, I think the LIST_BLOCKS functionality in kudu-fs_list should be preserved, because there isn't a 1-1 relationship between files and blocks, and so finding all blocks (scoped to a tablet or in total) isn't trivial. PS1, Line 59: options Nit: change to 'functionality' http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/CMakeLists.txt File src/kudu/tools/CMakeLists.txt: PS1, Line 55: add_library(fs_tool fs_tool.cc) : target_link_libraries(fs_tool : gutil : kudu_common : server_common : consensus : tablet) Can you remove fs_tool altogether and move the needed functionality into the tool_action_* code files? Especially with the removal of fs_list-tool there's functionality in fs_tool that's no longer needed. http://gerrit.cloudera.org:8080/#/c/4305/1/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: PS1, Line 187: unique_ptr<Action> dump_fs_blocks = : ActionBuilder("tablet_blocks", &DumpFsTabletBlocks) : .Description("Dump the data of tablet") : .AddRequiredParameter({ "tablet_id", "tablet identifier" }) : .AddOptionalParameter("fs_wal_dir") : .AddOptionalParameter("fs_data_dirs") : .Build(); : : unique_ptr<Action> dump_fs_data = : ActionBuilder("tablet_data", &DumpFsTabletData) : .Description("Dump the data of tablet") : .AddRequiredParameter({ "tablet_id", "tablet identifier" }) : .AddOptionalParameter("fs_wal_dir") : .AddOptionalParameter("fs_data_dirs") : .Build(); : : unique_ptr<Action> dump_fs_meta = : ActionBuilder("tablet_meta", &DumpFsTabletMeta) : .Description("Dump the metadata of tablet") : .AddRequiredParameter({ "tablet_id", "tablet identifier" }) : .AddOptionalParameter("fs_wal_dir") : .AddOptionalParameter("fs_data_dirs") : .Build(); : : unique_ptr<Action> dump_fs_rowset = : ActionBuilder("rowset", &DumpFsTabletRowSet) : .Description("Dump the rowset of tablet") : .AddRequiredParameter({ "tablet_id", "tablet identifier" }) : .AddRequiredParameter({ "rowset_index", "rowset index" }) : .AddOptionalParameter("fs_wal_dir") : .AddOptionalParameter("fs_data_dirs") : .Build(); These are all scoped to a tablet, so I think they should be in the tablet mode, not fs. Given that, probably makes sense to leave dump_cfile as it was in fs and put the dump mode in tablet. You can move 'dump_wals' already in tablet into the new dump mode too. PS1, Line 220: unique_ptr<Action> dump_tree = : ActionBuilder("tree", &DumpFsTree) : .Description("Dump the tree of Kudu filesystem") : .AddOptionalParameter("fs_wal_dir") : .AddOptionalParameter("fs_data_dirs") : .Build(); Let's drop this one altogether since basic UNIX tools like find can do the same thing. PS1, Line 237: .AddAction(std::move(dump_fs_blocks)) : .AddAction(std::move(dump_fs_data)) : .AddAction(std::move(dump_fs_meta)) : .AddAction(std::move(dump_fs_rowset)) These are all scoped to the 'fs' mode, so let's drop the fs infix. PS1, Line 242: print_uuid For consistency, let's change this to dump_uuid. You'll probably need to update several files. -- To view, visit http://gerrit.cloudera.org:8080/4305 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1ec628b65613011d8c48b6239c13762276425966 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dinesh Bhat <din...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
