Adar Dembo has posted comments on this change. Change subject: KUDU-1308 [c++-client]: support tables with non-covering range partitions ......................................................................
Patch Set 2: (15 comments) I reviewed everything but the change to flex_partitioning-itest.cc; will look at that in the next pass. http://gerrit.cloudera.org:8080/#/c/3255/2/src/kudu/client/client-internal.cc File src/kudu/client/client-internal.cc: Line 140: vector<uint32_t> required_feature_flags) { This is never used? http://gerrit.cloudera.org:8080/#/c/3255/2/src/kudu/client/client-test.cc File src/kudu/client/client-test.cc: Line 383: // Creates a table with 'num_replicas', split into tablets based on 'split_rows' : // (or single tablet if 'split_rows' is empty). Update this comment. Line 1045: KuduPartialRow* a_lower_bound = schema_.NewRow(); Nit: to be completely safe, we ought to store these in unique_ptrs, then release() them into CreateTable(). You might find this to be too verbose, but I thought I'd suggest it anyway. Line 1061: ASSERT_NO_FATAL_FAILURE(InsertTestRows(table.get(), 50, 0)); : client_->data_->meta_cache_->ClearCacheForTesting(); : ASSERT_NO_FATAL_FAILURE(InsertTestRows(table.get(), 50, 50)); : client_->data_->meta_cache_->ClearCacheForTesting(); : ASSERT_NO_FATAL_FAILURE(InsertTestRows(table.get(), 100, 200)); : client_->data_->meta_cache_->ClearCacheForTesting(); What's the significance of clearing the cache? Add a comment? Line 1068: // Insert an out-of-range rows Nit: no need for 'an'. Line 1084: Status result = session->Flush(); If you're going to flush with each row, how about using KuduSession::AUTO_FLUSH_SYNC? Line 1140: { Nit: one-line comments for these test cases too? http://gerrit.cloudera.org:8080/#/c/3255/2/src/kudu/client/client.h File src/kudu/client/client.h: Line 368: Add a partition range bound to the table with an inclusive lower bound and : // exclusive upper bound. I thought we were going to offer inclusive upper bounds? That's what I remember from your design doc. http://gerrit.cloudera.org:8080/#/c/3255/2/src/kudu/client/meta_cache.cc File src/kudu/client/meta_cache.cc: Line 755: if (tablets_by_key.empty()) { Should we do this before upper_bound(), since it doesn't depend on its result? Line 756: return Status::NotFound("No tablet covering the requested range partition"); Nit: you're creating the same Status in three different places. Perhaps use a bool to track the fact, then create it just once? Would be nice to emit the partition key into the status too, if that's not hard. Line 758: // The requested partition key is before all tablets Nit: end with period. Do the same for the others here, please. Line 762: *remote_tablet = itr->second; Is the dereference on the RHS safe? Does itr == begin() imply that there's an element (i.e. does it also imply itr != end())? Line 763: std::prev(itr)->second->partition().partition_key_end() > rpc.partition_key() || : std::prev(itr)->second->partition().partition_key_end().empty()) Likewise, are these std::prev() calls safe? What if itr == end()? Is a previous element guaranteed to exist? Line 811: ts_cache_.clear(); This doesn't leak the map values? http://gerrit.cloudera.org:8080/#/c/3255/2/src/kudu/client/scanner-internal.cc File src/kudu/client/scanner-internal.cc: Line 308: else if (!s.ok()) { : return s; : } Can do RETURN_NOT_OK(s) here instead. -- To view, visit http://gerrit.cloudera.org:8080/3255 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1cb12704c5e9792ee6e5831568bc52b1a713f8d5 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Dan Burkert <d...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
