Dan Burkert has posted comments on this change.

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

Patch Set 6:


File src/kudu/client/client.cc:

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 r

File src/kudu/master/catalog_manager.cc:

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?
It will be pretty verbose, and I don't think it's too useful at this point 
(experience may prove me wrong, though).

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 th
This is erasing by iterator, not by key, so it can't fail (and doesn't return 
an int).

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

File src/kudu/master/catalog_manager.h:

Line 207:   // Adds all tablets to the return vector in partition key sorted 
> Nit: returned vector
I really meant 'return' here, although I see how it's confusing, so I just 
removed 'return' altogether.

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 http://gerrit.cloudera.org:8080/3648
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 6
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: Jean-Daniel Cryans <jdcry...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-HasComments: Yes

Reply via email to