Adar Dembo has posted comments on this change.

Change subject: KUDU-1308 [c++-client]: support tables with non-covering range 

Patch Set 2:


I reviewed everything but the change to; will look 
at that in the next pass.
File src/kudu/client/

Line 140:     vector<uint32_t> required_feature_flags) {
This is never used?
File src/kudu/client/

Line 383: // Creates a table with 'num_replicas', split into tablets based on 
        :   // (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 

Line 1140:   {
Nit: one-line comments for these test cases too?
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.
File src/kudu/client/

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 
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() ||
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?
File src/kudu/client/

Line 308: else if (!s.ok()) {
        :       return s;
        :     }
Can do RETURN_NOT_OK(s) here instead.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I1cb12704c5e9792ee6e5831568bc52b1a713f8d5
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <>
Gerrit-HasComments: Yes

Reply via email to