Dan Burkert 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 
Done


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


Line 250:                                           vector<pair<string, 
string>>* range_partitions) const {
> To Todd's suggestions, perhaps we should DCHECK(range_partitions->empty()),
Done


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


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


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 g
I think we have it both ways, I'll switch this case.


Line 95:                                        const 
vector<tuple<KuduPartialRow,
> Can we use pair here instead of tuple? I agree with Todd that for 2-tuples,
Done


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


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


-- 
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 <d...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to