Adar Dembo has posted comments on this change.

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


Patch Set 4:

(8 comments)

http://gerrit.cloudera.org:8080/#/c/3255/4/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

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();


http://gerrit.cloudera.org:8080/#/c/3255/4/src/kudu/integration-tests/flex_partitioning-itest.cc
File src/kudu/integration-tests/flex_partitioning-itest.cc:

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 
gone.
        :     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 
written.


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 http://gerrit.cloudera.org:8080/3255
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1cb12704c5e9792ee6e5831568bc52b1a713f8d5
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to