Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/8911 )
Change subject: Add 'kudu fs list' tool ...................................................................... Patch Set 2: (6 comments) http://gerrit.cloudera.org:8080/#/c/8911/2/src/kudu/tools/tool_action_fs.cc File src/kudu/tools/tool_action_fs.cc: http://gerrit.cloudera.org:8080/#/c/8911/2/src/kudu/tools/tool_action_fs.cc@580 PS2, Line 580: BlockInfoRow Further below I got confused for a second because InfoRow returns a row to add to a DataTable while BlockInfoRow adds the row to the DataTable (and returns nothing). Could you rename BlockInfoRow accordingly? Maybe "AddBlockInfoRow". http://gerrit.cloudera.org:8080/#/c/8911/2/src/kudu/tools/tool_action_fs.cc@612 PS2, Line 612: ," nit: missing space http://gerrit.cloudera.org:8080/#/c/8911/2/src/kudu/tools/tool_action_fs.cc@645 PS2, Line 645: if (tablet_ids.empty()) { : return table.PrintTo(cout); : } I think we can get rid of this and simplify a little bit since the for loop below will have 0 iterations if tablet_ids is empty. http://gerrit.cloudera.org:8080/#/c/8911/2/src/kudu/tools/tool_action_fs.cc@666 PS2, Line 666: if (requires_rowset_info) Would it be simpler to do if (!requires_rowset_info) { table.AddRow(InfoRow(TabletInfo, columns, tablet)); continue; } so the level of indendation is reduce and it's more obvious that every below in the loop body depends on rowset info? http://gerrit.cloudera.org:8080/#/c/8911/2/src/kudu/tools/tool_action_fs.cc@674 PS2, Line 674: if (requires_block_info) ditto (mutatis mutandis) http://gerrit.cloudera.org:8080/#/c/8911/2/src/kudu/tools/tool_action_fs.cc@703 PS2, Line 703: should the tablet's orphaned blocks be included I'd say only by setting a flag to show them -- To view, visit http://gerrit.cloudera.org:8080/8911 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I7f5a63e636d95e3ee55bb4955cece7f5d0b7532d Gerrit-Change-Number: 8911 Gerrit-PatchSet: 2 Gerrit-Owner: Dan Burkert <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 22 Dec 2017 17:46:36 +0000 Gerrit-HasComments: Yes
