Start work on a minimal admin client. #93

coryan · 2017-12-12T13:10:25Z

This is part of the fixes for #79. While this is not needed for the MVP milestone it might help us with some of our integration tests. In this PR we just introduce the AdminClient and TableAdmin classes, and TableAdmin has a single ListTables member function.

Future PRs will introduce additional member functions to implement the full API for TableAdmin.

mbrukman · 2017-12-12T18:04:31Z

bigtable/admin/admin_client.cc

+  mutable std::mutex mu_;
+  std::shared_ptr<grpc::Channel> channel_;
+  std::unique_ptr<
+      google::bigtable::admin::v2::BigtableTableAdmin::StubInterface>


Be consistent whether this is google::[...] or ::google::[...] — it's different even in this one file.

mbrukman · 2017-12-12T18:04:33Z

bigtable/admin/admin_client.cc

+
+  std::string const& project() const override { return project_; }
+  void on_completion(grpc::Status const& status) override;
+  ::google::bigtable::admin::v2::BigtableTableAdmin::StubInterface&


Be consistent whether this is google::[...] or ::google::[...] — it's different even in this one file.

Or the next line! Thanks, fixed.

mbrukman · 2017-12-12T18:05:15Z

bigtable/admin/admin_client.cc

+}
+
+void SimpleAdminClient::refresh_credentials_and_channel() {
+  std::unique_lock<std::mutex> lk(mu_);


Are we going to use std::mutex or absl::Mutex in this library?

std::mutex, it is good enough for everybody.

mbrukman · 2017-12-13T05:39:26Z

bigtable/admin/admin_client.cc

+ private:
+  std::string project_;
+  bigtable::ClientOptions options_;
+  mutable std::mutex mu_;


Please add GUARDED_BY annotations (via Abseil's thread_annotations.h) to ensure that:

it's clear what member vars are guarded by mu_ and

the locks are enforced at compile time

Done. But there are bigger problems here.

mbrukman · 2017-12-13T05:43:30Z

bigtable/admin/admin_client.cc

+  if (table_admin_stub_) {
+    return;
+  }
+  // Release the lock before executing potentially slow operations.


Rather than doing that, would it make sense to create separate blocks, and use std::unique_lock and let its destructor automatically unlock, so you don't also have to relock? E.g., something like:

{ std::unique_lock lk(mu_); // some code under mutex here } // some slow code here { std::unique_lock lk(mu_); // some more code under mutex here }

It might read better (potentially), you skip the manual unlock/lock steps. Is constructing a new std::unique_lock expensive?

Thoughts?

mbrukman · 2017-12-13T05:44:01Z

bigtable/admin/admin_client_test.cc

@@ -0,0 +1,30 @@
+//   Copyright 2017 Google Inc.


Too many spaces after the // comment in this file for some reason.

mbrukman · 2017-12-13T05:45:15Z

bigtable/admin/table_admin_test.cc

@@ -0,0 +1,137 @@
+//   Copyright 2017 Google Inc.


Too many spaces after // in this file as well.

Fixed. How did that happen?

mbrukman · 2017-12-13T05:53:16Z

bigtable/client/client_options_test.cc

  EXPECT_EQ(typeid(grpc::GoogleDefaultCredentials()),
            typeid(client_options_object.credentials()));
 }

 TEST(ClientOptionsTest, ClientOptionsCustomEndpoint) {
+  // TODO(#23) - setenv() is a Unix specific call ...
  setenv("BIGTABLE_EMULATOR_HOST", "testendpoint.googleapis.com", 1);


Have to be really careful here: if this test ends too early (due to ASSERT_* being used instead of EXPECT_* at some point in the future, for example, this env var will stick around, affecting other tests, and making it quite hard to debug.

A way to guarantee that this happens is to create a custom test fixture class, where the SetUp() method does the setenv() call and TearDown() does the unsetenv() call. Definitely comes with some overhead, but will ensure that each test is isolated from other tests. It might be possible to write a single test fixture class, and then reuse it several times (via inheritance or template instantiation) so that you don't have to write this many times, but it's quite straight-forward and will save headaches in the future, I think.

This doesn't need to be done in this PR (since it's slightly a side comment, and the setenv() is from an earlier PR; please consider adding a TODO and/or filing an issue for this.

mbrukman · 2017-12-13T05:59:35Z

bigtable/client/client_options_test.cc

  EXPECT_EQ(typeid(grpc::GoogleDefaultCredentials()),
            typeid(client_options_object.credentials()));
 }

 TEST(ClientOptionsTest, ClientOptionsCustomEndpoint) {
+  // TODO(#23) - setenv() is a Unix specific call ...


Abseil uses:

#ifdef WIN32 SetEnvironmentVariable(...); #else setenv(...); #endif

Maybe it's worthwhile to start building a "common" library within google-cloud-cpp that will contain utility functions across products, such as a platform abstraction library and include such methods?

Created #100 to track this.

coryan · 2017-12-14T12:06:44Z

PTAL. Rebased, addressed comments, and opened bug to do the thread safety right the next time.

Also add a whopper of a TODO() item.

@mbrukman

Good catch @mbrukman!

Add TODO with link to a proper bug.

mbrukman · 2017-12-14T04:56:01Z

bigtable/admin/table_admin_test.cc

+  auto actual = tested.ListTables(btproto::Table::FULL);
+  std::string instance_name = static_cast<std::string>(tested.instance_name());
+  ASSERT_EQ(2UL, actual.size());
+  EXPECT_EQ(instance_name + "/tables/t0", actual[0].name());


Using std::string::operator+() in a few tests is probably OK, but for non-test code, we should probably be using abseil::StrCat(), I hear it's faster. :-)

mbrukman · 2017-12-14T15:03:16Z

bigtable/admin/admin_client.cc

+#include <absl/memory/memory.h>
+
+namespace {
+//github.com/ An implementation of the bigtable::AdminClient interface.


This is basically an English version of the next line; maybe rewrite it in terms of what it can vs. cannot do, instead of the mechanics of how it did this?

mbrukman · 2017-12-14T15:03:59Z

bigtable/admin/table_admin.cc

+namespace bigtable {
+inline namespace BIGTABLE_CLIENT_NS {
+std::string TableAdmin::CreateInstanceName() const {
+  std::string result("projects/");


Please use abseil::StrCat here instead, without a temporary var.

That requires compiling abseil. Let me take a look, but it is not trivial.

Done. That function better be amazing fast, because it drags an interesting set of dependencies.

mbrukman · 2017-12-14T15:06:29Z

bigtable/client/client_options.h

+
+  //github.com/ Return the current endpoint for data RPCs.
+  const std::string& data_endpoint() const { return data_endpoint_; }
+  ClientOptions& SetDataEndpoint(std::string endpoint) {


Should this not be set_data_endpoint() since it's a trivial setter method? Same for set_admin_endpoint below.

CamelCase implies a non-trivial / expensive implementation.

Done.

And that is one of the wrong things with the Google Style Guide naming conventions, they leak details about the implementation in the name of the functions.

mbrukman · 2017-12-14T15:06:46Z

bigtable/client/client_options_test.cc

+class ClientOptionsEmulatorTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    // TODO(#100) - setenv() is a Unix specific call ...


mbrukman · 2017-12-14T15:07:24Z

bigtable/client/client_options_test.cc

+  void SetUp() override {
+    // TODO(#100) - setenv() is a Unix specific call ...
+    setenv("BIGTABLE_EMULATOR_HOST", "testendpoint.googleapis.com", 1);
+    previous_ = std::getenv("BIGTABLE_EMULATOR_HOST");


Don't you want to call getenv() before you call setenv()?

mbrukman · 2017-12-14T15:07:39Z

bigtable/client/client_options_test.cc

+    setenv("BIGTABLE_EMULATOR_HOST", "testendpoint.googleapis.com", 1);
+    previous_ = std::getenv("BIGTABLE_EMULATOR_HOST");
+  }
+  void TearDown() override { unsetenv("BIGTABLE_EMULATOR_HOST"); }


If previous_ is non-null, did you want to restore it here?

Double oops.

coryan · 2017-12-14T17:35:58Z

PTAL.

The last commit did not disable it, and my local testing failed because of cached results.

Caching hid this problem from me (again!).

I was not thinking when I removed it. Left a comment for future unthinking me.

coryan · 2017-12-14T20:19:30Z

Now for realsies. The builds passed. Note to self: do not add dependencies in the middle of a PR.
PTAL.

mbrukman · 2017-12-17T23:44:18Z

CMakeLists.txt

@@ -104,7 +104,7 @@ elseif ("${GOOGLE_CLOUD_CPP_GRPC_PROVIDER}" STREQUAL "package")
    add_subdirectory(googletest EXCLUDE_FROM_ALL)
 endif ()

-# Enable unit tests.
+# We need to enable testing in this directory so we can do a top-level `make test`.


Drop the "We need to" prefix, it's cleaner.

mbrukman · 2017-12-17T23:45:09Z

bigtable/admin/admin_client.cc

+ *
+ * This class should not be used by multiple threads, it makes no attempt to
+ * protect its critical sections.  While it is rare that the admin interface
+ * will be used by multiple threads we should use the same approach here and in


s/threads we/threads, we/

mbrukman · 2017-12-17T23:45:58Z

bigtable/admin/admin_client.cc

+  table_admin() override;
+
+ private:
+  void refresh_credentials_and_channel();


This seems like it's not a simple getter or setter (and it makes network requests!), so presumably, its name should be RefreshCredentialsAndChannel(), no?

mbrukman · 2017-12-18T00:18:25Z

bigtable/admin/table_admin_test.cc

+  return [expected_token, returned_token](
+      grpc::ClientContext* ctx, btproto::ListTablesRequest const& request,
+      btproto::ListTablesResponse* response) {
+    EXPECT_EQ("projects/the-project/instances/the-instance", request.parent());


This line and a line below have an implicit dependency on a constant "the-project" defined in the test fixture, and the constant "the-instance" as used in every test.

At a minimum, there should be a comment that these dependencies exist.

In addition, maybe the project and instance should be constants in this file outside of the test fixture class, so that they can be referenced from the test fixture, this lambda, and test cases symbolically to show the relationship? Thoughts?

Done.
Let me know if I missed something.

mbrukman · 2017-12-18T00:18:36Z

bigtable/admin/table_admin_test.cc

+
+    EXPECT_NE(nullptr, response);
+    auto& t0 = *response->add_tables();
+    t0.set_name("projects/the-project/instances/the-instance/tables/t0");


Ditto here and for t1 below.

mbrukman · 2017-12-18T00:36:09Z

bigtable/admin/table_admin_test.cc

+  auto actual = tested.ListTables(btproto::Table::FULL);
+  std::string instance_name = static_cast<std::string>(tested.instance_name());
+  ASSERT_EQ(4UL, actual.size());
+  EXPECT_EQ(instance_name + "/tables/t0", actual[0].name());


It's not quite clear, without reading create_list_tables_lambda(), why this is the case that there are 4 returned values, of which 2 are repeated, and may confuse future readers, because this might be technically invalid output to have repeated tables.

Also, at first, I expected "but-wait-theres-more" to be a table and appear in the output, since test inputs should be related to expected test outputs somehow, but that's not the case here (since those are just continuation tokens, not expected outputs).

Would it make sense to modify create_list_tables_lambda() to accept a list of tables to be returned as a parameter, so that there's something meaningful in the test body that appears in the output? This will also allow the tables to be unique.

mbrukman · 2017-12-18T01:09:28Z

bigtable/client/client_options_test.cc

+class ClientOptionsEmulatorTest : public ::testing::Test {
+ protected:
+  void SetUp() override {
+    // TODO(#100) - setenv() is a Unix specific call.


Same comment in TearDown()?

mbrukman · 2017-12-18T01:10:01Z

bigtable/client/client_options_test.cc

  EXPECT_EQ(typeid(grpc::InsecureChannelCredentials()),
            typeid(client_options_object.credentials()));
-  unsetenv("BIGTABLE_EMULATOR_HOST");
 }

 TEST(ClientOptionsTest, EditEndpoint) {


s/EditEndpoint/EditDataEndpoint/ for clarity.

mbrukman · 2017-12-18T01:11:01Z

bigtable/doc/Doxyfile

@@ -119,7 +119,7 @@ WARN_LOGFILE           =
 # Configuration options related to the input files
 #---------------------------------------------------------------------------

-INPUT                  = README.md doc client
+INPUT                  = README.md doc client admin


Technically, both Data API and Admin API interfaces are "clients". While earlier, having just one client (for data) meant that client = "data client" and that was OK, now that you're adding an admin client, having client = "data client" while admin = "admin client" is a bit confusing.

Can we make this explicit and have a data_client and admin_client?

Created #103 to track this. I would prefer not to make another big change in this PR.

mbrukman · 2017-12-18T01:12:25Z

cmake/IncludeGMock.cmake

+        PUBLIC ${PROJECT_THIRD_PARTY_DIR}/googletest/googletest
+        PUBLIC ${PROJECT_THIRD_PARTY_DIR}/googletest/googlemock/include
+        PUBLIC ${PROJECT_THIRD_PARTY_DIR}/googletest/googlemock)
+


Extra blank line?

mbrukman

LGTM, one minor comment below. Let me know if you want me to re-review after you address it.

mbrukman · 2017-12-18T22:35:15Z

bigtable/admin/table_admin_test.cc

+  };
+  EXPECT_CALL(*table_admin_stub_, ListTables(_, _, _))
+      .WillOnce(Invoke(mock_unrecoverable_failure));
+  // We expect the TableAdmin to make 5 calls and to let the client know about


The "5" in this line is in stark contrast to the "1" on the next line — should this be updated?

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

coryan requested review from sduskis, mbrukman and paprat December 12, 2017 13:10

mbrukman suggested changes Dec 13, 2017

View reviewed changes

coryan force-pushed the minimal-admin-client branch from 67da59d to 036f7d7 Compare December 14, 2017 12:05

coryan and others added 18 commits December 14, 2017 07:10

WIP: create admin client [skip ci]

8797238

Got the code to compile.

c1cf735

Fix clang-tidy builds.

d2d013e

Add missing include.

e52b9f2

Fix dependencies for bigtable_admin_client library.

0fcdd23

Fix formatting with clang-format.

5db156d

Minor comments.

8ddac5f

Start implementation of TableAdmin.

831a6fc

Initial implement of ListTables().

bc5ea58

Reformat with clang-format.

f17e910

Fix doxygen docs.

f851726

Implement test for failure cases.

5c82c01

General cleanup before sending PR.

f028bf2

Fix formatting.

c7609f8

Address comments from review.

b9edec4

Also add a whopper of a TODO() item.

Use fixture to cleanup properly on failure.

28bc048

Good catch @mbrukman!

Use separate bug to track setenv() portability.

affef04

Remove the bad attempt at thread-safety.

2b58421

Add TODO with link to a proper bug.

coryan force-pushed the minimal-admin-client branch from 036f7d7 to 2b58421 Compare December 14, 2017 12:11

Fix formatting.

621f633

mbrukman suggested changes Dec 14, 2017

View reviewed changes

coryan added 2 commits December 14, 2017 10:31

Address most comments from review.

b4f4616

Compile absl, cctz, gmock to get absl::StrCat().

e774786

coryan added 4 commits December 14, 2017 13:14

Disable testing in cctz.

0600cb1

The last commit did not disable it, and my local testing failed because of cached results.

Fix include path for abseil.

a577cef

Caching hid this problem from me (again!).

Fix formatting.

b8c3ace

Restore top-level test.

5191320

I was not thinking when I removed it. Left a comment for future unthinking me.

Improve documentation for SimpleAdminClient.

23748fa

mbrukman suggested changes Dec 18, 2017

View reviewed changes

coryan mentioned this pull request Dec 18, 2017

Rename bigtable/client directory to bigtable/data #103

Closed

Address comments from review.

3003a09

mbrukman approved these changes Dec 18, 2017

View reviewed changes

Address comments from review [skip ci]

8e87373

coryan merged commit c19f9cb into googleapis:master Dec 18, 2017

coryan deleted the minimal-admin-client branch December 18, 2017 22:49

Start work on a minimal admin client. #93

Start work on a minimal admin client. #93

Uh oh!

Conversation

coryan commented Dec 12, 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!

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 commented Dec 14, 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!

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 commented Dec 14, 2017

Uh oh!

coryan commented Dec 14, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!