Adar Dembo has posted comments on this change.

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

Patch Set 10:

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?
File src/kudu/common/

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 
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.
File src/kudu/common/partition.h:

Line 257: 
Nit: unnecessary new line?
File src/kudu/integration-tests/

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 
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);
File src/kudu/master/

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

  KuduPartialRow a_lower(&kTableSchema);

To view, visit
To unsubscribe, visit

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

Reply via email to