Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11665 )
Change subject: [tools] Add a describe table tool ...................................................................... Patch Set 9: (5 comments) http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/client/client.h File src/kudu/client/client.h: http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/client/client.h@593 PS9, Line 593: friend Status tools::ListPartitions( : const client::sp::shared_ptr<client::KuduTable>& table, : std::vector<Partition>* partitions); This is no longer the preferred way to add private APIs. Instead, take the example of GetHiveMetastoreUris(): actually add a public function to KuduClient (or KuduTable, or whatever) but annotate it with KUDU_NO_EXPORT. http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/tools/kudu-admin-test.cc File src/kudu/tools/kudu-admin-test.cc: http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/tools/kudu-admin-test.cc@1379 PS9, Line 1379: "(\n" : " key INT32 NOT NULL,\n" : " int_val INT32 NOT NULL,\n" : " string_val STRING NULLABLE,\n" : " PRIMARY KEY (key)\n" : ")\n" : "RANGE (key) (\n" : " PARTITION UNBOUNDED" : "\n" : ")\n" : "REPLICAS 1"); Could avoid the excess double quotes (and simplify the formatting somewhat) with raw string literals. See https://en.cppreference.com/w/cpp/language/string_literal. Below too. http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/tools/tool_action_common.cc File src/kudu/tools/tool_action_common.cc: http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/tools/tool_action_common.cc@861 PS9, Line 861: // The strategy for retrieving the partitions from the metacache is adapted : // from KuduScanTokenBuilder::Data::Build. Would it be possible to share code between the two methods? Seems like you could encapsulate the bulk of the logic in a "for each partition" method that takes an std::function and calls it in the while loop after LookupTabletByKey(). http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/tools/tool_action_table.cc File src/kudu/tools/tool_action_table.cc: http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/tools/tool_action_table.cc@147 PS9, Line 147: cout << "TABLE " << table_name << " " << schema.ToString() << endl; Do you really want to print this if ListPartitions() fails? http://gerrit.cloudera.org:8080/#/c/11665/9/src/kudu/tools/tool_action_table.cc@168 PS9, Line 168: partition_strs.push_back(std::move(range_partition_str)); Nit: emplace_back in new code. -- To view, visit http://gerrit.cloudera.org:8080/11665 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib02f2a94cf11d0e60b32cd85be920ca9b99dd977 Gerrit-Change-Number: 11665 Gerrit-PatchSet: 9 Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Attila Bukor <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Wed, 17 Oct 2018 16:42:01 +0000 Gerrit-HasComments: Yes
