Will Berkeley 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 Done 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) Somebody suggested this on the original review, IIRC. The issue is you can't keep the indentation if you use a raw literal, so it needs to be on one line or not indented. 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 I experimented with this and it just seemed a lot harder to follow, particularly the more complex and lengthier logic in the scan token builder. 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? Yup, it's still useful info and not contingent on the partitions to make sense. 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. Done -- 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 (120) Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-Comment-Date: Fri, 19 Oct 2018 17:30:50 +0000 Gerrit-HasComments: Yes
