Adar Dembo has posted comments on this change.

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

Patch Set 4:

File src/kudu/client/

Line 776: if (not_found) return Status::NotFound("No tablet covering the 
requested range partition");
        :   return Status::OK();
Nit: how about:

  return not_found ? Status::NotFound("...") : Status::OK();
File src/kudu/integration-tests/

Line 63:   HashPartitionOptions(vector<string> columns,
Do you really need the constructors for this and RangePartitionOptions? Can't 
get by with regular C++ struct initialization? e.g.

  HashPartitionOptions opts = { {"foo"}, 1 };

Line 185: // Tablets aren't always immediately dropped, spin until they are 
        :     for (int i = 1; CountTablets() > 0; i++) {
        :       CHECK_LE(i, 30);
        :       base::SleepForMilliseconds(10 * i);
        :     }
Why is this important for the correctness of these tests?

Line 249:     STLDeleteValues(&ts_map);
What's this doing here? The map is new.

Line 250: CHECK_OK(itest::CreateTabletServerMap(cluster_->master_proxy().get(),
        :                                           cluster_->messenger(),
        :                                           &ts_map));
        :     vector<tserver::ListTabletsResponsePB_StatusAndSchemaPB> tablets;
        :     CHECK_OK(ListTablets(ts_map.begin()->second, 
MonoDelta::FromSeconds(10), &tablets));
        :     return tablets.size();
Seems strange to do this via a tserver call instead of GetTableLocations() with 
no bounds to the master. If you do the latter, I bet you won't have to wait for 
tablets to actually die in DeleteTable() either.

Line 263:   Status InsertRows(const RangePartitionOptions& range_partition, 
int* row_count);
Nit: also add that on success, row_count contains the number of rows actually 

Line 297:   vector<pair<vector<int32_t>, vector<int32_t>>> default_bounds = { { 
{ 0 }, { 1000 } } };
Maybe should be static and declared as kDefaultBounds?

Line 552:       NO_FATALS(TestPartitionOptions(hash_option, range_option));
Does this do the right thing even though there are no NO_FATALS() wrappers 
within TestPartitionOptions itself?

To view, visit
To unsubscribe, visit

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

Reply via email to