Dan Burkert 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/2/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

Line 811: void MetaCache::ClearCacheForTesting() {
> Surprised ASAN didn't complain about this. More missing test coverage?
There are ASAN failures, but I haven't tracked them all down yet...


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

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


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
Didn't know that was an option.   TIL.


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


PS4, 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() 
What class is GetTableLocations defined on?


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


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


Line 552:       NO_FATALS(TestPartitionOptions(hash_option, range_option));
> Does this do the right thing even though there are no NO_FATALS() wrappers 
I'm not sure.  I made sure every call site of a helper function that has an 
assert is wrapped in NO_FATALS, but maybe this isn't the correct way


-- 
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