Adar Dembo has posted comments on this change.

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


Patch Set 11:

(8 comments)

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, INT64_MAX,
> Err... updated to INT64_MAX.
Why is INT64_MAX a better choice than -1? The advantage of -1 is that it's 
actually printed in the help as "-1", whereas INT64_MAX looks like some huge 
number that a user might think has meaning as a real number.


Line 384:            nullptr, // MetricRegistry
> I can take it out, but it looked like it was dumping some in-memory content
The MRS will be empty if the tablet isn't bootstrapped. So in effect, it's 
still doing the same thing as "dump rowset". Plus it's doing it in a poor way: 
it loads _all_ the data into an in-memory vector, and the data is loaded as 
debug strings, which aren't as useful as how "dump rowset" prints.


PS9, Line 521: }
             : 
> Done, doesn't the 4-space indent next-line apply here ?
Generally yes, but sometimes people deviate from that style to "pretty print" 
some stuff (like this), and when they do, I think it's better code etiquette to 
preserve that style, at least on a file-by-file basis.


PS9, Line 607: // Rowset index no
> Heh, somewhere came across in Strostrup book few weeks ago, this way of ini
Primitive types don't have constructors/destructors the way that classes and 
structs do. It's all just raw memory.


PS9, Line 679: 
> Done
Nope, not changed.


PS9, Line 721: 
> Should be 'replica' now.
Looks like you missed this one.


http://gerrit.cloudera.org:8080/#/c/4305/11/src/kudu/tools/tool_action_local_replica.cc
File src/kudu/tools/tool_action_local_replica.cc:

PS11, Line 637: tablet
replica


PS11, Line 645: tablet
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: 11
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