Complete implementation of bigtable::TableAdmin #142
Conversation
googlebot
added
the
cla: yes
carterpage
left a comment
carterpage
left a comment
There was a problem hiding this comment.
Nice work. A few nits here and there.
bigtable/admin/table_admin.cc
Outdated
|
|
||
| void TableAdmin::DeleteTable(std::string table_id) { | ||
| btproto::DeleteTableRequest request; | ||
| request.set_name(CreateTableName(table_id)); |
There was a problem hiding this comment.
Mentioned in the other review, but I find "CreateTableName" confusing vs "TableName". "Create" generally implies state changes.
bigtable/admin/table_admin.h
Outdated
|
|
||
| private: | ||
| /// Compute the fully qualified instance name. | ||
| std::string CreateInstanceName() const; |
bigtable/admin/table_admin.h
Outdated
| std::string CreateInstanceName() const; | ||
|
|
||
| /// Return the fully qualified name of a table in this object's instance. | ||
| std::string CreateTableName(std::string const& table_id) const { |
bigtable/admin/table_admin_test.cc
Outdated
| } | ||
| modifications { | ||
| id: 'bar' | ||
| update { gc_rule { max_age { seconds: 172800 }}} |
There was a problem hiding this comment.
Maybe make this different (eg 24h) than the other rule, to make sure the values don't get mixed up.
bigtable/tests/admin_test.cc
Outdated
| throw std::runtime_error(os.str()); | ||
| } | ||
| std::cout << "GetTable(table1) successful" << std::endl; | ||
|
|
There was a problem hiding this comment.
Do a paranoid check that ListTables now returns two tables, since you're about to check after delete that list returns 1 once again.
bigtable/tests/admin_test.cc
Outdated
| } | ||
| std::cout << "ListTables() after DeleteTable() successful" << std::endl; | ||
|
|
||
| using GC = bigtable::GcRule; |
There was a problem hiding this comment.
After all the state changes above, it takes too much effort to reason about the current state and how the below modification should change it.
Create a new table right above the ModifyColumnFamilies call to make it easier to see the before/after side-by-side.
There was a problem hiding this comment.
Done.
At least I tried to make it easier to read, let me know if it needs further work.
bigtable/tests/admin_test.cc
Outdated
| #include "bigtable/admin/admin_client.h" | ||
| #include "bigtable/admin/table_admin.h" | ||
|
|
||
| int main(int argc, char* argv[]) try { |
There was a problem hiding this comment.
This isn't super extensible as a one-method test. I'd suggest breaking it into two methods you invoke from main, first one testing create+get+delete operations, and the second testing column family modification.
It would be nice if there were a comment with simple description of the test. Even "Integration test to create and delete some tables" is sufficient.
|
|
||
| echo | ||
| echo "Running TableAdmin integration test." | ||
| ./admin_test emulated admin-tests |
There was a problem hiding this comment.
File names are confusing. They are both integration tests. Can we change them to data_integration_test and admin_integration_test or at least something consistent?
|
PTAL. |
This is part of the changes for googleapis#79. This completes the class, though we need a few more integration tests.
Great suggestion, thanks @carterpage.
coryan
force-pushed
the
minimal-admin-client-t6
branch
from
35f2c12 to
8ee2015
Compare
3 participants
This fixes #79. In this PR we introduced the remaining TableAdmin functions, and provide an integration test for the class. There are missing integration tests for DropRows*, because their implementation would be overly complicated until #32 is done. There is a separate issue (#141) to track this.