Dinesh Bhat has posted comments on this change. Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool ......................................................................
Patch Set 9: (12 comments) http://gerrit.cloudera.org:8080/#/c/4305/8/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: PS8, Line 114: > In the latest diff (PS9) it's still there. Done PS8, Line 476: : // TODO: it's awkward that whenever we want to iterate over deltas we also : // need to open the CFileSet for the rowset. Ide > Yes, but you didn't update the comment to mention unique_ptr instead of gsc Done PS8, Line 682: .AddOptionalParameter("fs_data_dirs") > But the two aren't equivalent. "All rowsets" means all on-disk rowsets, whi What I meant was 'dump blocks' was dumping all rowsets for a tablet_id, only diff between the two actions were row_index, hence combined them now. http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/tool_action_local_replica.cc File src/kudu/tools/tool_action_local_replica.cc: Line 70: DEFINE_int64(rowset_index, 0, "Index of the rowset in local replica"); > But rowset index 0 is a valid index. You should default to -1 here, and mod Yeah, plus checking with if(FLAGS_rowset_index) would yield unexpected results as well. I am using INT_MAX as default value instead of -1. Updated help accordingly. PS9, Line 139: fs_ptr->reset(new FsManager(Env::Default(), fs_opts)); : RETURN_NOT_OK((*fs_ptr)->Open()); > Nit: generally, we prefer to use the calling convention where the OUT param Done Line 384: Status DumpData(const RunnerContext& context) { > I took a look at how this is implemented, and I think we should remove it t I can take it out, but it looked like it was dumping some in-memory contents too(MRS), and wasn't sure about removing it. Perhaps we can consolidate 'dump data' and 'dump rowset' post 1.0 ? I didn't want to remove altogether and re-add later if we find something missing. PS9, Line 434: bool metadata_only > Every time this function is called, FLAGS_metadata_only is fed into this ar Done PS9, Line 521: cout << Indent(indent) << upd.key.ToString() << " " : << RowChangeList(upd.cell).ToString(schema) << endl; > The old code (before removing the std:: prefixes) took care to align the << Done, doesn't the 4-space indent next-line apply here ? PS9, Line 607: bool found{false}; > What's this? Why not "bool found = false;"? Heh, somewhere came across in Strostrup book few weeks ago, this way of initializing is supported for built-in types too, but I am curious if the code in 'found = false' would resort to invoking constructor too for built-in types. May be not. PS9, Line 667: tablet > Replica. Done PS9, Line 669: .AddOptionalParameter("rowset_index") > Resort this list of parameters alphabetically. Done PS9, Line 679: tablet > Should now be 'replica'. Done -- 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: 9 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
