Dan Burkert has posted comments on this change.

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

Patch Set 4:


File src/kudu/client/client.cc:

PS4, Line 892:     
             :         data_->client_, alter_name, deadline));
> If this fails, don't we want to clear the meta cache anyway? Maybe ClearCac
It can actually happen before waiting for the reasons outlined below.  Moved.

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

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
DropRange then AddRange is legal if the range already exists, and effectively 
truncates the existing range, but the new range is still subject to the 
visibility caveats.  AddRange then DropRange is legal if the range doesn't 
already exist, and is effectively a no-op (the tablet shouldn't even be 
created).  And finally, any combination of these is possible, e.g. Add -> Drop 
-> Add -> Drop, etc.

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/meta_cache.cc:

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 c

File src/kudu/integration-tests/alter_table-randomized-test.cc:

Line 363:     LOG(INFO) << "Adding column " << name;
> Were these LOG statements just for debugging or do you want to keep them?
They are really helpful in debugging the test, and they don't happen too often 
(insert/update/delete are common, but set to vlog).

Line 459:       scanner.SetTimeoutMillis(60000);
> What's the significance of this value?
This was copied from ScanTableToStrings, which wasn't suitable anymore because 
it doesn't use a fault tolerant scanner (sort order is important).

Line 510:   vector<string> master_addrs_;
> Unused?

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 AlterTa

File src/kudu/integration-tests/alter_table-test.cc:

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?
It's copied from InsertRows above.

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?
Yes, this method assumes the test class's schema_ as the schema.

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

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

Line 1065:   gscoped_ptr<KuduTableAlterer> table_alterer;
> unique_ptr?

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

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 
yah, I changed this up slightly since that's not really worth testing as it was.

File src/kudu/master/catalog_manager.cc:

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 o

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, righ

Line 1259:           auto iter = tablets.upper_bound(lower_bound);
> Can't use FindFloorOrNull here and below?
Because the iterator is moved backwards in the check below (line 1273), which I 
don't think is safe with a vanilla pointer.

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?
yes, however it got a little more complicated in order to support this in the 
latest revision.

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 i
yes, otherwise the messages would need to be copied.

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 ScopedTable
They are built up in ApplyAlterPartitioningSteps, and there are a lot of ways 
that can fail.  If I understand correctly, they really don't become important 
until step 7 when the sys catalog is updated, and at that point they are added 
to a committer.

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

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 wi

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?

Line 1540:     for (const auto& tablet : tablets_to_drop) {
> Since we're leaving these tablets behind in tablet_map_ (as we should), we 
We discussed this, and it involved changing how tablets are revealed slightly.

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 als

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

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