Todd Lipcon 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 Done PS3, Line 2273: may possibly > Why "may possibly"? Done Line 2280: /// the row to be partitioned. > Nit: capital letter (below too). Done PS3, Line 2286: partitions > not "columns"? Done Line 2294: Data* data_; > Nit: add // Owned. comment. Done 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 KuduPartiti right. 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. Done Line 55: std::map<std::string, int> partitions_by_start_key_; > Why not unordered_map? because the keys we're looking up fall between partitions, so we need sorted-order. It might be faster to use a boost flat_map, or a trie, or something like that here for better CPU cache locality. But sticking with the simplest to start. -- 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
