Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list (WIP)

Patch Set 1:


TFTR Adar, updated the patch, please re-review, also added some more tests. I 
think I can keep adding some more test exercises as you review them.

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 functional
Added that back, although I have kept some of the sub commands like 
tree/list_tablets as well now since it's useful to pretty print them instead of 
relying on 'find'. Also please find some routines like ListLogSegments etc 
which I think could be removed. I will do so after taking some confirmation 
from your end.

PS1, Line 59: options
> Nit: change to 'functionality'

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 th
I have done this, but not exactly as you described. I moved pretty much 
everything under tool_action_common(as opposed to moving just common routines), 
reason being I thought these subcommands may go through another round of 
re-shuffle after feedbacks from dev/users, we could start reorganizing them at 
that point. For now, FsTool simply moved from fs_tool library to 
tool_action_common.* and got rid of fs_tool library as such.

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 m
Done, kept the dump_wals as-is since it seemed to be taking tablet_id as param.

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 
Hmmm, I kept this only for pretty printing purposes, kinda looks nice with 
hierarchies enforced by indentations.

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 up

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: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to