Adar Dembo has posted comments on this change. Change subject: KUDU-1713: add a client Partitioner API ......................................................................
Patch Set 3: (8 comments) http://gerrit.cloudera.org:8080/#/c/5775/3/src/kudu/client/client.h File src/kudu/client/client.h: Line 2240: /// the resulting KuduPartitioner instance Add that the caller takes ownership (see some of the other doxygen comments in this file for inspiration). Also, convention is to begin the sentence with a capital letter and close with a period. I think. PS3, Line 2273: may possibly Why "may possibly"? Line 2280: /// the row to be partitioned. Nit: capital letter (below too). PS3, Line 2286: partitions not "columns"? Line 2294: Data* data_; Nit: add // Owned. comment. http://gerrit.cloudera.org:8080/#/c/5775/3/src/kudu/client/partitioner-internal.cc File src/kudu/client/partitioner-internal.cc: Line 78: *partition = FindFloorOrDie(partitions_by_start_key_, tmp_buf_); This will never actually "die", right? I mean, barring a bug in KuduPartitionerBuilder::Data::Build()? http://gerrit.cloudera.org:8080/#/c/5775/3/src/kudu/client/partitioner-internal.h File src/kudu/client/partitioner-internal.h: Line 39: CHECK(timeout.Initialized()); Since this is a library, better to return a bad Status than to crash. Line 55: std::map<std::string, int> partitions_by_start_key_; Why not unordered_map? -- To view, visit http://gerrit.cloudera.org:8080/5775 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ic08a078c75f15ef4200219b5260cfb99d79b72cc Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Todd Lipcon <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Matthew Jacobs <[email protected]> Gerrit-Reviewer: Thomas Tauber-Marshall <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
