Dan Burkert has posted comments on this change.

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


Patch Set 4:

(37 comments)

http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.cc
File src/kudu/client/client.cc:

PS4, Line 892:     
RETURN_NOT_OK(data_->client_->data_->WaitForAlterTableToFinish(
             :         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".
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/client.h
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"?
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/meta_cache.cc
File src/kudu/client/meta_cache.cc:

Line 19: 
> What were the include and using changes for?
mistake.


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/client/table_alterer-internal.h
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
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-randomized-test.cc
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?
Done


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
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/integration-tests/alter_table-test.cc
File src/kudu/integration-tests/alter_table-test.cc:

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


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


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?
Done


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
Done


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


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


PS4, Line 1091:   table_alterer->wait(true);
              :   
ASSERT_OK(table_alterer->AddRangePartition(add_lower.release(), 
add_upper.release())->Alter());
> Combine?
Done


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.


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/catalog_manager.cc
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;
              :   
RETURN_NOT_OK(PartitionSchema::FromPB(table->metadata().state().pb.partition_schema(),
              :                                         schema, 
&partition_schema));
> This isn't safe; you should be getting the schema() through the COWLocked o
Done


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
Done


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> 
metadata(&cow_metadata,
              :                                                    
CowLock<PersistentTabletInfo>::READ);
> This is super funky. Why can't you just use a TabletMetadataLock?
Done


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.
Done


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 
Done


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
Done


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


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


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.
Done


Line 1546:     l.Commit();
> Does this need to be done with lock_ held? It didn't previously. It can als
Done


Line 1547:     SendAlterTableRequest(table);
> Don't do this with lock_ held.
Done


http://gerrit.cloudera.org:8080/#/c/3648/4/src/kudu/master/master.proto
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.
Done


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