Dinesh Bhat has posted comments on this change.

Change subject: cli tool: List all tablets/replica_uuids with 'kudu table list'
......................................................................


Patch Set 2:

(7 comments)

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

PS1, Line 10: correlat
> correlate
Done


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: false,
> Nit: don't need this breadcrumb. I only added it for timeout_ms because tha
Done


Line 178:   unique_ptr<ConsensusMetadata> cmeta;
> I'd rather we didn't do this; it makes the output less machine-readable. Or
Alrighty, as discussed offline, removed them.


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

PS1, Line 81:     cout << tname << endl;
            :     if (!FLAGS_list_tablets) {
            :       continue;
            :     }
            :     client::sp::shared_ptr<KuduTable> client_table;
            :     RETURN_NOT_OK(client->OpenTable(tname,
> All this should happen after the verbose check, since its output isn't need
Done


PS1, Line 89:     KuduScanTokenBuilder builder(client_table.get());
            :     RETURN_NOT_OK(builder.Build(&tokens));
            : 
> Won't this cause just the first table to be printed?
Good catch, looks like I was testing with one table too :)


PS1, Line 95:         cout << "P" << (replica->is_leader() ? "(L) " : " ")
            :              << replica->ts().uuid() << " ";
> To avoid leaking too many Raft implementation details, let's  annotate the 
Done


Line 124:       .AddOptionalParameter("list_tablets")
> Printing extra information during "kudu table list" is useful, but I'd pref
done, i pinged in hipchat before seeing this comment of yours, happy that we 
arrived at same conclusion. i replaced '--list-tablets' or suggest with any 
other choices.


-- 
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: 2
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: Dinesh Bhat <din...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to