Dinesh Bhat has posted comments on this change.

Change subject: tool: port kudu-fs_dump, remove kudu-fs_list, fs_tool

Patch Set 9:


File src/kudu/tools/tool_action_local_replica.cc:

PS8, Line 114: 
> In the latest diff (PS9) it's still there.

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

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.

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 

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

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

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.

PS9, Line 669:       .AddOptionalParameter("rowset_index")
> Resort this list of parameters alphabetically.

PS9, Line 679: tablet
> Should now be 'replica'.

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

Reply via email to