Implement CreateTable() for the admin client #117

coryan · 2017-12-19T20:50:01Z

This is another PR in the series to fix #79. It implements the CreateTable() function.

mbrukman · 2017-12-19T20:52:10Z

bigtable/admin/table_admin.cc

+      break;
+    }
+    if (not rpc_policy->on_failure(status)) {
+      throw std::runtime_error("could not create table");


It doesn't tell me which table it had trouble creating.

mbrukman · 2017-12-19T20:53:15Z

bigtable/admin/table_admin_test.cc

  EXPECT_THROW(tested.ListTables(btproto::Table::FULL), std::exception);
 }
+
+/// @test Verify that bigtable::TableAdmin::Create works in the easy case.


bigtable::TableAdmin::Create() <-- code font + () ?

mbrukman · 2017-12-19T20:56:27Z

bigtable/admin/table_admin_test.cc

+initial_splits { key: 'p' }
+)""";
+    btproto::CreateTableRequest expected;
+    EXPECT_TRUE(google::protobuf::TextFormat::ParseFromString(expected_text,


ASSERT_TRUE? If it can't parse it, might as well fail now. Similarly elsewhere.

mbrukman · 2017-12-19T20:57:03Z

bigtable/admin/table_admin_test.cc

+    std::string delta;
+    google::protobuf::util::MessageDifferencer differencer;
+    differencer.ReportDifferencesToString(&delta);
+    EXPECT_TRUE(differencer.Compare(expected, request)) << delta;


Is this the only thing we have for protobuf comparisons? There's no ASSERT_THAT gMock matcher support?

I couldn't find it; just asking if you know of anything that's more readable than MessageDifferencer.

I did look too, could not find anything in googletest, nor in protobuf. At least the output is readable with the MessageDifferencer, though yes, the test code is less than awesome.

mbrukman · 2017-12-19T21:45:37Z

bigtable/admin/table_admin.cc

+    auto delay = backoff_policy->on_completion(status);
+    std::this_thread::sleep_for(delay);
+  }
+  return response;


It's a bit unfortunate that we're returning a Table and not something like StatusOr, so we cannot propagate the error gracefully to the caller, just print it out to stderr.

Should we consider an exception-free implementation which returns the error as a Status? Or can that be the default implementation, and the exception version converts Status to an exception?

Created #119 for this. We can create our own exception that derives from std::exception and encapsulates the grpc::Status.

garye · 2017-12-19T22:31:17Z

bigtable/admin/table_admin.cc

+::google::bigtable::admin::v2::Table TableAdmin::CreateTable(
+    std::string table_id, std::map<std::string, GcRule> column_families,
+    std::vector<std::string> splits,
+    ::google::bigtable::admin::v2::Table::TimestampGranularity granularity) {


We only support millisecond granularity right now. At the very least we should document this, or perhaps not expose it at all.

Also, thoughts on creating a TableConfig or similar structure to make it easier to add new table params in the future?

I think we should expose the API to set the granularity because if we don't then when we do it is a backwards incompatible change that would require a new major version number. Though maybe it would require that anyway.

Not opposed to TableConfig and in this case probably makes sense, will update this PR tomorrow.

garye · 2017-12-19T22:35:00Z

bigtable/admin/table_admin.cc

+    if (status.ok()) {
+      break;
+    }
+    if (not rpc_policy->on_failure(status)) {


What happens if the original requests succeeds but a transient error is returned, followed by an ALREADY_EXISTS error? Probably should not throw an exception.

Hmm... there is a certain amount of ad-hoc logic that would be needed. We would need to query the table to return something meaningful. Created #121 for it because we will need to implement GetTable() to fix this. And that is coming in a future PR already.

mbrukman · 2017-12-20T20:47:10Z

bigtable/admin/table_config.h

+        granularity_(TIMESTAMP_GRANULARITY_UNSPECIFIED) {}
+
+  /// Move the contents to the proto to create tables.
+  void MoveTo(::google::bigtable::admin::v2::CreateTableRequest& request);


Google C++ style guide suggests (recommends?) using pointers for output params.

mbrukman · 2017-12-20T20:48:38Z

bigtable/admin/table_config_test.cc

+  config.set_timestamp_granularity(bigtable::TableConfig::MILLIS);
+
+  auto const& families = config.column_families();
+  ASSERT_NE(families.end(), families.find("fam"));


What about:

auto it = families.find("fam"); ASSERT_NE(families.end(), it); EXPECT_EQ(2, it->second...);

mbrukman

LGTM

All my concerns have been addressed (thanks!); over to @garye for approval.

mbrukman · 2017-12-21T00:13:52Z

@coryan — looks like builds are failing:

.../bigtable/admin/table_admin_test.cc:27:45: fatal error: bigtable/client/chrono_literals.h: No such file or directory
 #include "bigtable/client/chrono_literals.h"
                                             ^

Was this file moved to bigtable/client/test/* in this or another PR? I feel like I've seen that in one of the dozen PRs I reviewed recently. 😄

The ASSERT_* macros return void, they cannot be used in functions or lambdas that return something else.

Per the code review.

Also fixed some of the formatting problems, but running into issues with my version of clang-format.

And explain why in like 8 lines.

coryan · 2017-12-21T02:58:36Z

Needed to rebase and then fix a path. It was hilarious to get a clean merge that broken all the builds.

googlebot added the cla: yes This human has signed the Contributor License Agreement. label Dec 19, 2017

coryan requested review from garye and mbrukman December 19, 2017 20:50

mbrukman suggested changes Dec 19, 2017

View reviewed changes

mbrukman reviewed Dec 19, 2017

View reviewed changes

garye suggested changes Dec 19, 2017

View reviewed changes

mbrukman reviewed Dec 20, 2017

View reviewed changes

mbrukman approved these changes Dec 20, 2017

View reviewed changes

garye approved these changes Dec 20, 2017

View reviewed changes

coryan added 13 commits December 20, 2017 20:44

Implement CreateTable()

9967bb9

Expand integration test to create and list tables.

1fb1127

Fixed doxygen docs.

880a7c9

Address comments from review.

3dd7982

Move ASSERT_TRUE() outside mock implementations.

f1a4b27

The ASSERT_* macros return void, they cannot be used in functions or lambdas that return something else.

Add TableConfig class.

64b406d

Per the code review.

Use TableConfig in CreateTable.

7809e61

Address comments from review.

921bf91

Also fixed some of the formatting problems, but running into issues with my version of clang-format.

Fix formatting error.

592e2ac

Address comments from review.

099c1ae

Disable clang-tidy warnings in two lines.

a175fa2

And explain why in like 8 lines.

Fix formatting.

8c9f07a

Add fix syntax error.

b06abe1

coryan force-pushed the minimal-admin-client-t3 branch from 5498d64 to b06abe1 Compare December 21, 2017 01:45

Change include path after rebase.

e95f99c

coryan merged commit 255497c into googleapis:master Dec 21, 2017

coryan deleted the minimal-admin-client-t3 branch December 21, 2017 02:54

mbrukman mentioned this pull request Dec 24, 2017

Create a client library for the Admin API. #79

Closed

Implement CreateTable() for the admin client #117

Implement CreateTable() for the admin client #117

Uh oh!

Conversation

coryan commented Dec 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

coryan Dec 19, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mbrukman left a comment

Choose a reason for hiding this comment

Uh oh!

mbrukman commented Dec 21, 2017

Uh oh!

coryan commented Dec 21, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

coryan Dec 19, 2017 •

edited

Loading

coryan Dec 19, 2017 •

edited

Loading