Adar Dembo has posted comments on this change.

Change subject: KUDU-1311 [master] support adding and dropping range partitions

Patch Set 6:

File src/kudu/client/

PS6, Line 853: KuduPartialRow* lower_bound,
KuduPartialRow* upper_bound
Should we first check that the bounds aren't null?
File src/kudu/integration-tests/external_mini_cluster.h:

Line 406:   Status WaitForMasterUpAndRunning() WARN_UNUSED_RESULT;
Maybe doc this new method? And since it's an ExternalMaster method, maybe 
rename to "WaitForCatalogManager" or something more specific?
File src/kudu/master/

Line 1345:   }
> yes, otherwise the messages would need to be copied.
Can't these vectors be of crefs instead? I just don't think any other methods 
here modify the input, and it doesn't seem like something we should do.
File src/kudu/master/

PS6, Line 1267:               return Status::NotFound("New partition conflicts 
with existing partition",
              :                                       step.DebugString());
Can we point out the conflict in these messages? Or is that too verbose?

PS6, Line 1272:             auto prev = std::prev(existing_iter);
              :             TabletMetadataLock metadata(prev->second, 
Nit: combine?

Line 1284:             if (metadata.pb.partition().partition_key_start()< 
upper_bound) {
Nit: partition_key_start() < upper_bound

Line 1331:             existing_tablets.erase(existing_iter);
Maybe CHECK that this returns 1? Or add EraseOrDie to map-util.h and use that?

Line 1551:   {
Please add a comment here rationalizing the order of operations.

Line 1563:     table->AddRemoveTablets(tablets_to_add, tablets_to_drop);
I don't remember why we thought this should be done with the global catalog 
manager spinlock held. Can you remind me?
File src/kudu/master/catalog_manager.h:

Line 207:   // Adds all tablets to the return vector in partition key sorted 
Nit: returned vector

Line 230:   std::map<std::string, TabletInfo*> tablet_map() const {
Let's make the TabletInfoMap typedef public and use it in this method.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 6
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Dan Burkert <>
Gerrit-Reviewer: Adar Dembo <>
Gerrit-Reviewer: Dan Burkert <>
Gerrit-Reviewer: Jean-Daniel Cryans <>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to