Skip to content

Commit 6647a6a

Browse files
scotthartscotthart
committed
address review comments and build errors
1 parent 7450a4a commit 6647a6a

File tree

3 files changed

+32
-38
lines changed

3 files changed

+32
-38
lines changed

‎google/cloud/bigtable/internal/query_plan.cc‎

Lines changed: 9 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -140,12 +140,11 @@ void QueryPlan::RefreshQueryPlan(RefreshMode mode, Status error) {
140140
}
141141
}
142142

143-
StatusOr<QueryPlan::ResponseData> QueryPlan::response_data() {
143+
StatusOr<google::bigtable::v2::PrepareQueryResponse> QueryPlan::response() {
144144
std::unique_lock<std::mutex> lock(mu_);
145145
if (IsRefreshing(lock)) {
146146
if (response_.ok()) {
147-
return QueryPlan::ResponseData{response_->prepared_query(),
148-
response_->metadata()};
147+
return response_;
149148
}
150149
lock.unlock();
151150
RefreshQueryPlan(RefreshMode::kAlreadyRefreshing);
@@ -156,20 +155,19 @@ StatusOr<QueryPlan::ResponseData> QueryPlan::response_data() {
156155
return response_.status();
157156
}
158157

159-
return QueryPlan::ResponseData{response_->prepared_query(),
160-
response_->metadata()};
158+
return response_;
161159
}
162160

163161
StatusOr<std::string> QueryPlan::prepared_query() {
164-
auto data = response_data();
165-
if (!data.ok()) return std::move(data.status());
166-
return std::move(data->prepared_query);
162+
auto data = response();
163+
if (!data.ok()) return data.status();
164+
return response_->prepared_query();
167165
}
168166

169167
StatusOr<google::bigtable::v2::ResultSetMetadata> QueryPlan::metadata() {
170-
auto data = response_data();
171-
if (!data.ok()) return std::move(data.status());
172-
return std::move(data->metadata);
168+
auto data = response();
169+
if (!data.ok()) return data.status();
170+
return response_->metadata();
173171
}
174172

175173
GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_END

‎google/cloud/bigtable/internal/query_plan.h‎

Lines changed: 4 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -45,19 +45,14 @@ class QueryPlan : public std::enable_shared_from_this<QueryPlan> {
4545
// Invalidates the current QueryPlan and triggers a refresh.
4646
void Invalidate(Status status, std::string const& invalid_query_plan_id);
4747

48-
struct ResponseData {
49-
std::string prepared_query;
50-
google::bigtable::v2::ResultSetMetadata metadata;
51-
};
52-
5348
// Accessor for the prepared_query and metadata fields in response_.
5449
// Triggers a refresh if needed.
55-
StatusOr<ResponseData> response_data();
50+
StatusOr<google::bigtable::v2::PrepareQueryResponse> response();
5651

57-
GOOGLE_CLOUD_CPP_DEPRECATED("Use response_data() instead")
52+
GOOGLE_CLOUD_CPP_DEPRECATED("Use response() instead")
5853
StatusOr<std::string> prepared_query();
5954

60-
GOOGLE_CLOUD_CPP_DEPRECATED("Use response_data() instead")
55+
GOOGLE_CLOUD_CPP_DEPRECATED("Use response() instead")
6156
StatusOr<google::bigtable::v2::ResultSetMetadata> metadata();
6257

6358
private:
@@ -90,6 +85,7 @@ class QueryPlan : public std::enable_shared_from_this<QueryPlan> {
9085
// State machine where the only valid transitions are:
9186
// kDone -> kBegin
9287
// kBegin -> kPending
88+
// kPending -> kBegin
9389
// kPending -> kDone
9490
// When refreshing the same previous query plan.
9591
enum class RefreshState {

‎google/cloud/bigtable/internal/query_plan_test.cc‎

Lines changed: 19 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -65,10 +65,10 @@ TEST(QueryPlanTest, ResponseDataWithOriginalValidQueryPlan) {
6565
make_status_or(google::bigtable::v2::PrepareQueryResponse{}));
6666
});
6767

68-
auto response_data = plan->response_data();
68+
auto response_data = plan->response();
6969
ASSERT_STATUS_OK(response_data);
70-
EXPECT_EQ(response_data->prepared_query, "test-query");
71-
EXPECT_THAT(response_data->metadata, IsProtoEqual(metadata));
70+
EXPECT_EQ(response_data->prepared_query(), "test-query");
71+
EXPECT_THAT(response_data->metadata(), IsProtoEqual(metadata));
7272

7373
// Cancel all pending operations, satisfying any remaining futures.
7474
fake_cq_impl->SimulateCompletion(false);
@@ -94,16 +94,16 @@ TEST(QueryPlanTest, RefreshExpiredPlan) {
9494
auto query_plan = QueryPlan::Create(CompletionQueue(fake_cq_impl), response,
9595
refresh_fn, fake_clock);
9696

97-
auto data = query_plan->response_data();
97+
auto data = query_plan->response();
9898
ASSERT_STATUS_OK(data);
99-
EXPECT_EQ(data->prepared_query, "original-query-plan");
99+
EXPECT_EQ(data->prepared_query(), "original-query-plan");
100100

101101
fake_clock->AdvanceTime(std::chrono::seconds(500));
102102
fake_cq_impl->SimulateCompletion(true);
103103

104-
data = query_plan->response_data();
104+
data = query_plan->response();
105105
ASSERT_STATUS_OK(data);
106-
EXPECT_EQ(data->prepared_query, "refreshed-query-plan");
106+
EXPECT_EQ(data->prepared_query(), "refreshed-query-plan");
107107

108108
// Cancel all pending operations, satisfying any remaining futures.
109109
fake_cq_impl->SimulateCompletion(false);
@@ -129,14 +129,14 @@ TEST(QueryPlanTest, FailedRefreshExpiredPlan) {
129129
auto query_plan = QueryPlan::Create(CompletionQueue(fake_cq_impl), response,
130130
refresh_fn, fake_clock);
131131

132-
auto data = query_plan->response_data();
132+
auto data = query_plan->response();
133133
ASSERT_STATUS_OK(data);
134-
EXPECT_EQ(data->prepared_query, "original-query-plan");
134+
EXPECT_EQ(data->prepared_query(), "original-query-plan");
135135

136136
fake_clock->AdvanceTime(std::chrono::seconds(500));
137137
fake_cq_impl->SimulateCompletion(true);
138138

139-
data = query_plan->response_data();
139+
data = query_plan->response();
140140
EXPECT_THAT(data.status(), StatusIs(StatusCode::kInternal, "oops!"));
141141

142142
// Cancel all pending operations, satisfying any remaining futures.
@@ -163,16 +163,16 @@ TEST(QueryPlanTest, RefreshInvalidatedPlan) {
163163
auto query_plan = QueryPlan::Create(CompletionQueue(fake_cq_impl), response,
164164
refresh_fn, fake_clock);
165165

166-
auto data = query_plan->response_data();
166+
auto data = query_plan->response();
167167
ASSERT_STATUS_OK(data);
168-
EXPECT_EQ(data->prepared_query, "original-query-plan");
168+
EXPECT_EQ(data->prepared_query(), "original-query-plan");
169169

170170
auto invalid_status = internal::InternalError("oops!");
171-
query_plan->Invalidate(invalid_status, data->prepared_query);
171+
query_plan->Invalidate(invalid_status, data->prepared_query());
172172

173-
data = query_plan->response_data();
173+
data = query_plan->response();
174174
ASSERT_STATUS_OK(data);
175-
EXPECT_EQ(data->prepared_query, "refreshed-query-plan");
175+
EXPECT_EQ(data->prepared_query(), "refreshed-query-plan");
176176

177177
// Cancel all pending operations, satisfying any remaining futures.
178178
fake_cq_impl->SimulateCompletion(false);
@@ -198,14 +198,14 @@ TEST(QueryPlanTest, FailedRefreshInvalidatedPlan) {
198198
auto query_plan = QueryPlan::Create(CompletionQueue(fake_cq_impl), response,
199199
refresh_fn, fake_clock);
200200

201-
auto data = query_plan->response_data();
201+
auto data = query_plan->response();
202202
ASSERT_STATUS_OK(data);
203-
EXPECT_EQ(data->prepared_query, "original-query-plan");
203+
EXPECT_EQ(data->prepared_query(), "original-query-plan");
204204

205205
auto invalid_status = internal::InternalError("oops!");
206-
query_plan->Invalidate(invalid_status, data->prepared_query);
206+
query_plan->Invalidate(invalid_status, data->prepared_query());
207207

208-
data = query_plan->response_data();
208+
data = query_plan->response();
209209
EXPECT_THAT(data.status(), StatusIs(StatusCode::kInternal, "oops again!"));
210210

211211
// Cancel all pending operations, satisfying any remaining futures.

0 commit comments

Comments
 (0)