Adar Dembo has posted comments on this change.

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


Patch Set 9:

(14 comments)

http://gerrit.cloudera.org:8080/#/c/4305/9/src/kudu/tools/kudu-tool-test.cc
File src/kudu/tools/kudu-tool-test.cc:

PS9, Line 687: kRownId
kRowId


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: 
> Agreed, done.
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
> Done
Yes, but you didn't update the comment to mention unique_ptr instead of 
gscoped_ptr.


PS8, Line 682:       .AddOptionalParameter("fs_data_dirs")
> Very interesting catch ! it turns out dump blocks was nothing but dumping a
But the two aren't equivalent. "All rowsets" means all on-disk rowsets, which 
means all on-disk data blocks.


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 modify 
the help text to explain the special value.


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 parameter 
(fs_ptr in this case) isn't modified unless the function succeeds. So what you 
should do is store the new FsManager in a local unique_ptr, then after Open() 
succeeds, swap the local unique_ptr with fs_ptr.


Line 384: Status DumpData(const RunnerContext& context) {
I took a look at how this is implemented, and I think we should remove it too. 
The problem is that it purports to dumping all of the data of the tablet, but 
it doesn't actually bootstrap the tablet, so any data in WAL segments is 
missed. After that, it's functionally equivalent to DumpRowSet (with 
rowset_index==0) in that it'll just dump all the on-disk blocks.


PS9, Line 434: bool metadata_only
Every time this function is called, FLAGS_metadata_only is fed into this 
argument, so just drop the argument and access FLAGS_metadata_only directly 
inside the function.


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 << 
that begins each line. Could you do the same?

This comment applies elsewhere in this file too.


PS9, Line 607: bool found{false};
What's this? Why not "bool found = false;"?


PS9, Line 667: tablet
Replica.

Please make the same changes fo rthe other dump Descriptions.


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


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


PS9, Line 721: tablet
Should be 'replica' now.


-- 
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