Adar Dembo has posted comments on this change.

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

Patch Set 4:

File src/kudu/client/

PS4, Line 892:     
             :         data_->client_, alter_name, deadline));
If this fails, don't we want to clear the meta cache anyway? Maybe ClearCache() 
should be set up as a ScopedCleanup thing right after L888-889.

Line 908:     // It is sufficient to clear that cache even if the table 
alteration is not
Nit: "the cache".

Also, not sure about "sufficient"; it implies that there was something more we 
could do but have chosen not to. Maybe you meant "necessary"?
File src/kudu/client/client.h:

PS4, Line 537: unless the existing range partitions are dropped first
What happens if a single KuduTableAlterer has DropRange and AddRange on the 
same range? Is that legal, provided they're in that order?

Line 541:   // this method returns, however other existing clients may have to 
wait for a
Don't you mean "when Alter() returns success"?
File src/kudu/client/

Line 19: 
What were the include and using changes for?
File src/kudu/client/table_alterer-internal.h:

Line 57:   struct PartitioningStep {
I think the code would be net less complex if PartitionStep and Step were 
combined. The overhead of two unique_ptrs per Step for non-partitioning steps 
is minimal, and it'll simplify ToRequest() somewhat.
File src/kudu/integration-tests/

Line 363:     LOG(INFO) << "Adding column " << name;
Were these LOG statements just for debugging or do you want to keep them?

Line 459:       scanner.SetTimeoutMillis(60000);
What's the significance of this value?

Line 510:   vector<string> master_addrs_;

PS4, Line 547:     } else if (r < 850 && t.num_range_partitions() < 
kMaxRangePartitions) {
             :       t.AddRangePartition();
             :     } else if (r < 900) {
             :       t.DropRangePartition();
It would be cool if this also tested "compound" operations, i.e. an 
AlterTable() that adds a partition, drops another, and adds a column too.
File src/kudu/integration-tests/

Line 27: #include "kudu/client/client.h"
Nit: the previous sort order was more correct.

Line 36: #include "kudu/master/master-test-util.h"
Nit: likewise, this was more correct before.

Line 463:   session->SetTimeoutMillis(15 * 1000);
Why this value?

Line 468:     gscoped_ptr<KuduInsert> insert(table->NewInsert());
Use unique_ptr here?

PS4, Line 470:     if (table->schema().num_columns() > 1) {
             :       RETURN_NOT_OK(insert->mutable_row()->SetInt32(1, i));
             :     }
The assumption being that every column is going to be an Int32?

Line 550: int AlterTableTest::CountRows(const string& table_name) {
Can you reuse CountTableRows() from client-test-util.h? There may be some other 
useful goodies there.

Line 1064: TEST_F(AlterTableTest, TestAlterRangePartitioning) {
How about a test case for one AlterTable() adding and dropping the same 

Also, what about negative test cases? Like dropping partitions that don't 
exist, adding partitions that overlap with existing ones, adding the same 
partition twice, etc.

Line 1065:   gscoped_ptr<KuduTableAlterer> table_alterer;

PS4, Line 1091:   table_alterer->wait(true);

PS4, Line 1097:   ASSERT_OK(InsertRowsSequential(table_name, 0, 100));
              :   ASSERT_EQ(100, CountRows(table_name));
This is to test that IsAlterTableInProgress()->false actually means we can 
File src/kudu/master/

PS4, Line 1215:   Schema schema;
RETURN_NOT_OK(SchemaFromPB(table->metadata().state().pb.schema(), &schema));
              :   PartitionSchema partition_schema;
              :                                         schema, 
This isn't safe; you should be getting the schema() through the COWLocked 

PS4, Line 1223:     std::lock_guard<simple_spinlock> l(table->lock_);
              :     tablets = table->tablet_map_;
This is why you needed to add CatalogManager as a friend of TableInfo, right? 
Instead, can you add a method to get a copy of the TableInfo's TabletMap?

Line 1259:           auto iter = tablets.upper_bound(lower_bound);
Can't use FindFloorOrNull here and below?

PS4, Line 1264:             CowLock<PersistentTabletInfo> 
This is super funky. Why can't you just use a TabletMetadataLock?

PS4, Line 1287: tablets.emplace(lower_bound, tablets_to_add->back().get());
This is so that a DROP can remove a tablet added by a preceding ADD?

Line 1337:   vector<AlterTableRequestPB::Step> alter_schema_steps;
Nit: add this step to the overall numbering of steps in this function.

Line 1345:         alter_schema_steps.emplace_back(std::move(step));
Doesn't the std::move() here modify req->alter_schema_steps(). since step is a 
non-const ref? That's odd; why do that?

PS4, Line 1443:   vector<scoped_refptr<TabletInfo>> tablets_to_add;
              :   vector<scoped_refptr<TabletInfo>> tablets_to_drop;
What about removing these and putting the tablets directly into 
ScopedTabletInfoCommitters? It'd be a LOCKED one for added tablets and UNLOCKED 
for dropped. You'd need to add a line to abort the new committer around L1513, 
but it'd mean less state (and thus complexity) in this function.

Line 1517:   tablets_to_drop_committer.Commit();
Hmm, I wonder if this should be made visible only _after_ the tablets have been 
removed from the table.

Line 1519:   // 8. Remove the old name and add the new name.
Can we combine this with the lock_ acquisition below so that it's atomic with 
respect to the other AlterTable changes?

Line 1536:       tablet->mutable_metadata()->CommitMutation();
These can be done outside of lock_, right?

Line 1537:       table->AddTablet(tablet.get());
I thought we were going to do table->AddTablets() to add them all en masse?

For that matter, shouldn't the addition and subtraction of tablets in the table 
be done atomically with respect to the table's lock?

Line 1540:     for (const auto& tablet : tablets_to_drop) {
Since we're leaving these tablets behind in tablet_map_ (as we should), we need 
to make sure they're not revealed in GetTabletLocations(). Can you make that 
change and add a unit test to prove that has the right semantics?

Line 1543:       SendDeleteTabletRequest(tablet, l, deletion_msg);
Also shouldn't be done with lock_ held.

Line 1546:     l.Commit();
Does this need to be done with lock_ held? It didn't previously. It can also be 
conditionalized on has_schema_changes, though we should probably Abort() (to 
release the lock) before calling SendAlterTableRequest.

Line 1547:     SendAlterTableRequest(table);
Don't do this with lock_ held.
File src/kudu/master/master.proto:

PS4, Line 436:     ADD_RANGE_PARTITION=5;
             :     DROP_RANGE_PARTITION=6;
Nit: add space before and after the equals signs.

To view, visit
To unsubscribe, visit

Gerrit-MessageType: comment
Gerrit-Change-Id: I42437f365397baf9d4b39b5b17a1587fae70c4be
Gerrit-PatchSet: 4
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