Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(spanner): spanner::Client construction from Options #7706

Merged
merged 1 commit into from
Dec 7, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions google/cloud/spanner/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ add_library(
bytes.h
client.cc
client.h
client_options.cc
client_options.h
commit_options.h
commit_result.h
Expand Down Expand Up @@ -157,6 +158,7 @@ add_library(
partition_options.h
partitioned_dml_result.h
polling_policy.h
query_options.cc
query_options.h
query_partition.cc
query_partition.h
Expand Down
2 changes: 1 addition & 1 deletion google/cloud/spanner/client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -318,7 +318,7 @@ QueryOptions Client::OverlayQueryOptions(QueryOptions const& preferred) {
google::cloud::internal::GetEnv("SPANNER_OPTIMIZER_STATISTICS_PACKAGE"));

return spanner_internal::OverlayQueryOptions(
preferred, opts_.query_options(), *kOptimizerVersionEnvValue,
preferred, query_opts_, *kOptimizerVersionEnvValue,
*kOptimizerStatisticsPackageEnvValue);
}

Expand Down
30 changes: 27 additions & 3 deletions google/cloud/spanner/client.h
Original file line number Diff line number Diff line change
Expand Up @@ -36,13 +36,15 @@
#include "google/cloud/spanner/transaction.h"
#include "google/cloud/spanner/version.h"
#include "google/cloud/backoff_policy.h"
#include "google/cloud/internal/non_constructible.h"
#include "google/cloud/optional.h"
#include "google/cloud/options.h"
#include "google/cloud/status.h"
#include "google/cloud/status_or.h"
#include <google/spanner/v1/spanner.pb.h>
#include <grpcpp/grpcpp.h>
#include <functional>
#include <initializer_list>
#include <memory>
#include <string>
#include <vector>
Expand Down Expand Up @@ -128,10 +130,31 @@ class Client {
* with unit testing, callers may create fake/mock `Connection` objects that
* are injected into the `Client`.
*/
explicit Client(std::shared_ptr<Connection> conn, ClientOptions opts = {})
explicit Client(std::shared_ptr<Connection> conn, Options opts = {})
: conn_(std::move(conn)), opts_(std::move(opts)) {}

/// No default construction. Use `Client(std::shared_ptr<Connection>)`
/**
* Constructs a `Client` object using the specified @p conn and @p opts.
*
* Note that the previous constructor, which uses the new way to represent
* options of all varieties (`google::cloud::Options`), is now preferred.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mark as @deprecated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I planned to leave deprecating ClientOptions to a later stage.

explicit Client(std::shared_ptr<Connection> conn, ClientOptions const& opts)
: conn_(std::move(conn)), opts_(opts) {}

/**
* Constructs a `Client` object using the specified @p conn and an empty
* initializer list.
*
* @note This constructor exists solely for backwards compatibility. It
* prevents existing code that uses `Client(conn, {})` from breaking
* due to ambiguity introduced by the `Options` overload.
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In pubsub, I introduced this constructor as @deprecated, to scare away any users reading the documentation. In the long run, we do want to remove the function, but, we also want Client(conn, {}) to be valid code that calls the Options constructor. (So I'm not sure using @deprecated was correct).

Changing from a @note to a @warning is also possible. I'll let you decide if you think either of these are improvements.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't think @deprecated is best for the reasons you cite. And indeed, @warning falls to the same logic methinks. The @note is really just an answer to "What's this?", and it all goes away when ClientOptions does.

explicit Client(std::shared_ptr<Connection> conn,
std::initializer_list<internal::NonConstructible>)
: Client(std::move(conn)) {}

/// No default construction.
Client() = delete;

//@{
Expand Down Expand Up @@ -648,7 +671,8 @@ class Client {
QueryOptions OverlayQueryOptions(QueryOptions const&);

std::shared_ptr<Connection> conn_;
ClientOptions opts_;
Options opts_;
QueryOptions query_opts_{opts_};
};

/**
Expand Down
49 changes: 49 additions & 0 deletions google/cloud/spanner/client_options.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
// Copyright 2021 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "google/cloud/spanner/client_options.h"
#include "google/cloud/spanner/options.h"

namespace google {
namespace cloud {
namespace spanner {
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN

ClientOptions::operator Options() const {
Options opts;
auto optimizer_version = query_options_.optimizer_version();
if (optimizer_version) {
opts.set<RequestOptimizerVersionOption>(*optimizer_version);
}
auto optimizer_statistics_package =
query_options_.optimizer_statistics_package();
if (optimizer_statistics_package) {
opts.set<RequestOptimizerStatisticsPackageOption>(
*optimizer_statistics_package);
}
auto request_priority = query_options_.request_priority();
if (request_priority) {
opts.set<RequestPriorityOption>(*request_priority);
}
auto request_tag = query_options_.request_tag();
if (request_tag) {
opts.set<RequestTagOption>(*request_tag);
}
return opts;
}

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace spanner
} // namespace cloud
} // namespace google
7 changes: 7 additions & 0 deletions google/cloud/spanner/client_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

#include "google/cloud/spanner/query_options.h"
#include "google/cloud/spanner/version.h"
#include "google/cloud/options.h"

namespace google {
namespace cloud {
Expand All @@ -35,6 +36,12 @@ class ClientOptions {
ClientOptions(ClientOptions&&) = default;
ClientOptions& operator=(ClientOptions&&) = default;

/**
* Convert the `ClientOptions` to the new, recommended way to represent
* options of all varieties, `google::cloud::Options`.
*/
explicit operator Options() const;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in pubsub a conversion method like this was hidden away in internal with the logic being that ClientOptions was to be deprecated, so no public methods should be added to it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking that a customer might want to call this as part of their own migration, but I'm just guessing. In any case, with ClientOptions on the road to removal I'm not too concerned about adding to the API.


/// Returns the `QueryOptions`
QueryOptions const& query_options() const { return query_options_; }

Expand Down
29 changes: 29 additions & 0 deletions google/cloud/spanner/client_options_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

#include "google/cloud/spanner/client_options.h"
#include "google/cloud/spanner/options.h"
#include "google/cloud/spanner/query_options.h"
#include "google/cloud/spanner/version.h"
#include <gmock/gmock.h>
Expand All @@ -37,6 +38,34 @@ TEST(ClientOptionsTest, OptimizerVersion) {
EXPECT_EQ(copy, default_constructed);
}

TEST(ClientOptionsTest, OptionsConversionEmpty) {
ClientOptions const client_options;
auto const options = Options(client_options);
EXPECT_FALSE(options.has<RequestOptimizerVersionOption>());
EXPECT_FALSE(options.has<RequestOptimizerStatisticsPackageOption>());
EXPECT_FALSE(options.has<RequestPriorityOption>());
EXPECT_FALSE(options.has<RequestTagOption>());
}

TEST(ClientOptionsTest, OptionsConversionFull) {
auto query_options = QueryOptions{}
.set_optimizer_version("1")
.set_optimizer_statistics_package("latest")
.set_request_priority(RequestPriority::kHigh)
.set_request_tag("tag");
ClientOptions client_options;
client_options.set_query_options(std::move(query_options));
auto const options = Options(client_options);
EXPECT_TRUE(options.has<RequestOptimizerVersionOption>());
EXPECT_EQ(options.get<RequestOptimizerVersionOption>(), "1");
EXPECT_TRUE(options.has<RequestOptimizerStatisticsPackageOption>());
EXPECT_EQ(options.get<RequestOptimizerStatisticsPackageOption>(), "latest");
EXPECT_TRUE(options.has<RequestPriorityOption>());
EXPECT_EQ(options.get<RequestPriorityOption>(), RequestPriority::kHigh);
EXPECT_TRUE(options.has<RequestTagOption>());
EXPECT_EQ(options.get<RequestTagOption>(), "tag");
}

} // namespace
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace spanner
Expand Down
2 changes: 2 additions & 0 deletions google/cloud/spanner/google_cloud_cpp_spanner.bzl
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,7 @@ google_cloud_cpp_spanner_srcs = [
"backup.cc",
"bytes.cc",
"client.cc",
"client_options.cc",
"connection_options.cc",
"database.cc",
"database_admin_client.cc",
Expand Down Expand Up @@ -162,6 +163,7 @@ google_cloud_cpp_spanner_srcs = [
"mutations.cc",
"numeric.cc",
"partition_options.cc",
"query_options.cc",
"query_partition.cc",
"read_partition.cc",
"results.cc",
Expand Down
18 changes: 17 additions & 1 deletion google/cloud/spanner/options.h
Original file line number Diff line number Diff line change
Expand Up @@ -156,6 +156,22 @@ using SessionPoolOptionList = OptionList<
SessionPoolMaxIdleSessionsOption, SessionPoolActionOnExhaustionOption,
SessionPoolKeepAliveIntervalOption, SessionPoolLabelsOption>;

/**
* Option for `google::cloud::Options` to set the optimizer version used in an
* SQL query.
*/
struct RequestOptimizerVersionOption {
using Type = std::string;
};

/**
* Option for `google::cloud::Options` to set the optimizer statistics package
* used in an SQL query.
*/
struct RequestOptimizerStatisticsPackageOption {
using Type = std::string;
};

/**
* Option for `google::cloud::Options` to set a `spanner::RequestPriority`.
*/
Expand All @@ -171,7 +187,7 @@ struct RequestTagOption {
};

/**
* List of all Request options.
* List of Request options for `client::ExecuteBatchDml()`.
*/
using RequestOptionList = OptionList<RequestPriorityOption, RequestTagOption>;

Expand Down
42 changes: 42 additions & 0 deletions google/cloud/spanner/query_options.cc
Original file line number Diff line number Diff line change
@@ -0,0 +1,42 @@
// Copyright 2021 Google LLC
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

#include "google/cloud/spanner/query_options.h"
#include "google/cloud/spanner/options.h"

namespace google {
namespace cloud {
namespace spanner {
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN

QueryOptions::QueryOptions(Options const& opts) {
if (opts.has<RequestOptimizerVersionOption>()) {
optimizer_version_ = opts.get<RequestOptimizerVersionOption>();
}
if (opts.has<RequestOptimizerStatisticsPackageOption>()) {
optimizer_statistics_package_ =
opts.get<RequestOptimizerStatisticsPackageOption>();
}
if (opts.has<RequestPriorityOption>()) {
request_priority_ = opts.get<RequestPriorityOption>();
}
if (opts.has<RequestTagOption>()) {
request_tag_ = opts.get<RequestTagOption>();
}
}

GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace spanner
} // namespace cloud
} // namespace google
7 changes: 7 additions & 0 deletions google/cloud/spanner/query_options.h
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include "google/cloud/spanner/request_priority.h"
#include "google/cloud/spanner/version.h"
#include "google/cloud/optional.h"
#include "google/cloud/options.h"
#include "absl/types/optional.h"
#include <string>

Expand All @@ -41,6 +42,12 @@ class QueryOptions {
QueryOptions(QueryOptions&&) = default;
QueryOptions& operator=(QueryOptions&&) = default;

/**
* Construct from the the new, recommended way to represent options of all
* varieties, `google::cloud::Options`.
*/
explicit QueryOptions(Options const& opts);

/// Returns the optimizer version
absl::optional<std::string> const& optimizer_version() const {
return optimizer_version_;
Expand Down
27 changes: 27 additions & 0 deletions google/cloud/spanner/query_options_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
// limitations under the License.

#include "google/cloud/spanner/query_options.h"
#include "google/cloud/spanner/options.h"
#include "google/cloud/spanner/version.h"
#include <gmock/gmock.h>

Expand Down Expand Up @@ -83,6 +84,32 @@ TEST(QueryOptionsTest, OptimizerStatisticsPackage) {
EXPECT_EQ(copy, default_constructed);
}

TEST(QueryOptionsTest, FromOptionsEmpty) {
auto const opts = Options{};
QueryOptions const query_opts(opts);
EXPECT_FALSE(query_opts.optimizer_version());
EXPECT_FALSE(query_opts.optimizer_statistics_package());
EXPECT_FALSE(query_opts.request_priority());
EXPECT_FALSE(query_opts.request_tag());
}

TEST(QueryOptionsTest, FromOptionsFull) {
auto const opts = Options{}
.set<RequestOptimizerVersionOption>("1")
.set<RequestOptimizerStatisticsPackageOption>("latest")
.set<RequestPriorityOption>(RequestPriority::kHigh)
.set<RequestTagOption>("tag");
QueryOptions const query_opts(opts);
ASSERT_TRUE(query_opts.optimizer_version());
EXPECT_EQ(*query_opts.optimizer_version(), "1");
ASSERT_TRUE(query_opts.optimizer_statistics_package());
EXPECT_EQ(*query_opts.optimizer_statistics_package(), "latest");
ASSERT_TRUE(query_opts.request_priority());
EXPECT_EQ(*query_opts.request_priority(), RequestPriority::kHigh);
ASSERT_TRUE(query_opts.request_tag());
EXPECT_EQ(*query_opts.request_tag(), "tag");
}

} // namespace
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END
} // namespace spanner
Expand Down