[kudu-CR] KUDU-1307 [master] support tables with range partition bounds

["Adar Dembo (Code Review)" <ge...@cloudera.org>]

Adar Dembo has posted comments on this change.

Change subject: KUDU-1307 [master] support tables with range partition bounds
......................................................................


Patch Set 10:

(9 comments)

http://gerrit.cloudera.org:8080/#/c/2806/10//COMMIT_MSG
Commit Message:

Line 11: not covered by a tablet. A new feature master feature flag has been added so
"A new feature master feature flag" is a little awkward, is there an extra 'feature' here?


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/common/partition.cc
File src/kudu/common/partition.cc:

Line 241:     Arena arena(1024, 1024 * 1024);
What's this doing here?


Line 250:                                           vector<pair<string, string>>* range_partitions) const {
To Todd's suggestions, perhaps we should DCHECK(range_partitions->empty()), the same way you've done that for the other Encode functions?


Line 264:           "Range bound has lower bound equal to or above the upper bound",
So FWIW, I was wrong in telling you to use capital letters for the beginning of error messages. Todd reminded me of that later; since we append status messages to one another, it's best for them to start in lower-case so the composed sentences look less awkward. Sorry.


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/common/partition.h
File src/kudu/common/partition.h:

Line 257: 
Nit: unnecessary new line?


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/integration-tests/table_locations-itest.cc
File src/kudu/integration-tests/table_locations-itest.cc:

Line 43: namespace kudu {
Is this typical? Don't we prefer to put all the "using" statements in one giant block together, prefixing with kudu:: if necessary?


Line 95:                                        const vector<tuple<KuduPartialRow,
Can we use pair here instead of tuple? I agree with Todd that for 2-tuples, pairs are more intuitive.


Line 144:   KuduPartialRow row = KuduPartialRow(&schema);
Nit: KuduPartialRow row(&schema);


http://gerrit.cloudera.org:8080/#/c/2806/10/src/kudu/master/master-test.cc
File src/kudu/master/master-test.cc:

Line 411:     KuduPartialRow a_lower = KuduPartialRow(&kTableSchema);
Nit: these would be more terse without the copy constructor:

  KuduPartialRow a_lower(&kTableSchema);


-- 
To view, visit http://gerrit.cloudera.org:8080/2806
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I04a7ed3e85c993fdbcb313a419f15031103736ce
Gerrit-PatchSet: 10
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mp...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: Yes