Adar Dembo has posted comments on this change.

Change subject: cli tool: add -v to 'kudu table list'
......................................................................


Patch Set 1:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/4440/1//COMMIT_MSG
Commit Message:

PS1, Line 10: corelate
correlate


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

PS1, Line 67: // defined in tool_action_common
Nit: don't need this breadcrumb. I only added it for timeout_ms because that 
was defined in ksck, which is a separate "module" from the CLI tool.


Line 178:   cout << "List of replica uuids for tablet " << tablet_id << ":" << 
endl;
I'd rather we didn't do this; it makes the output less machine-readable. Or at 
least introduce a switch to control whether the output is human-readable or 
machine-readable.


http://gerrit.cloudera.org:8080/#/c/4440/1/src/kudu/tools/tool_action_table.cc
File src/kudu/tools/tool_action_table.cc:

Line 21: #include <iterator>
Don't need this anymore.


PS1, Line 34: // defined in tool_action_common
Don't need this.


PS1, Line 81:     client::sp::shared_ptr<KuduTable> client_table;
            :     RETURN_NOT_OK(client->OpenTable(tname, &client_table));
            :     vector<KuduScanToken*> tokens;
            :     ElementDeleter deleter(&tokens);
            :     KuduScanTokenBuilder builder(client_table.get());
            :     RETURN_NOT_OK(builder.Build(&tokens));
All this should happen after the verbose check, since its output isn't needed 
if !verbose.


PS1, Line 89:     if (!FLAGS_verbose) {
            :       return Status::OK();
            :     }
Won't this cause just the first table to be printed?


PS1, Line 95:         cout << "P" << (replica->is_leader() ? "(L) " : "(F) ")
            :              << replica->ts().uuid() << " ";
To avoid leaking too many Raft implementation details, let's  annotate the 
leader replica with (L) but let's not annotate the followers at all.


Line 124:       .AddOptionalParameter("verbose")
Printing extra information during "kudu table list" is useful, but I'd prefer 
if we didn't use 'verbose' for it. I was thinking we'd use 'verbose' to control 
glog output. Right now if we want users to see important output we have to 
redirect stderr to /dev/null, because there's enough INFO-level glog output 
that it muddies up the stdout output. In the future, we can hide all of that by 
default and use 'verbose' to turn it on when someone is debugging.

So let's replace this with something targeted for list_tables, such as 
"include_replicas" or some such.


-- 
To view, visit http://gerrit.cloudera.org:8080/4440
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Ic8f8e0dfb8e7ba9f67d5926199a9b831351585a7
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to