Skip to content

Commit a5e6a34

Browse files
mangchiandjjoemangchiandjjoe
authored
Removed pending requests management (istio#361)
* Removed pending requests management * Exposed ConfigLoadingStatus * Added description of the config loading status
1 parent 7d01533 commit a5e6a34

File tree

8 files changed

+23
-673
lines changed

8 files changed

+23
-673
lines changed

‎contrib/endpoints/include/api_manager/api_manager.h

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -87,6 +87,12 @@ class ApiManager {
8787
virtual utils::Status GetStatistics(
8888
ApiManagerStatistics *statistics) const = 0;
8989

90+
// Return the initialization status
91+
// - Code::UNAVAILABLE Not initialized yet. The default value.
92+
// - Code::OK Successfully initialized
93+
// - Code::ABORTED Initialization was failed
94+
virtual utils::Status ConfigLoadingStatus() const = 0;
95+
9096
protected:
9197
ApiManager() {}
9298

‎contrib/endpoints/src/api_manager/BUILD

Lines changed: 0 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -205,20 +205,6 @@ cc_test(
205205
],
206206
)
207207

208-
cc_test(
209-
name = "request_handler_test",
210-
size = "small",
211-
srcs = [
212-
"request_handler_test.cc",
213-
],
214-
linkstatic = 1,
215-
deps = [
216-
":api_manager",
217-
":mock_api_manager_environment",
218-
"//external:googletest_main",
219-
],
220-
)
221-
222208
cc_test(
223209
name = "config_manager_test",
224210
size = "small",

‎contrib/endpoints/src/api_manager/api_manager_impl.cc

Lines changed: 2 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -125,30 +125,17 @@ utils::Status ApiManagerImpl::Init() {
125125
if (rollouts.size() == 0) {
126126
config_loading_status_ =
127127
utils::Status(Code::ABORTED, "Invalid service config");
128-
129-
// Abort pending Call and Report callbacks
130-
NotifyPendingRequests(config_loading_status_);
131128
return;
132129
}
133130

134131
DeployConfigs(std::move(rollouts));
135132
}
136133
config_loading_status_ = status;
137-
138-
// Execute pending Call and Report callbacks
139-
NotifyPendingRequests(config_loading_status_);
140134
});
141135

142136
return utils::Status::OK;
143137
}
144138

145-
void ApiManagerImpl::NotifyPendingRequests(utils::Status status) {
146-
for (auto const &pending_callback : pending_request_callbacks_) {
147-
pending_callback(status);
148-
}
149-
pending_request_callbacks_.clear();
150-
}
151-
152139
utils::Status ApiManagerImpl::Close() {
153140
if (global_context_->cloud_trace_aggregator()) {
154141
global_context_->cloud_trace_aggregator()->SendAndClearTraces();
@@ -196,14 +183,6 @@ std::shared_ptr<context::ServiceContext> ApiManagerImpl::SelectService() {
196183
return nullptr;
197184
}
198185

199-
bool ApiManagerImpl::IsConfigLoadingInProgress() {
200-
return config_loading_status_.code() == Code::UNAVAILABLE;
201-
}
202-
203-
bool ApiManagerImpl::IsConfigLoadingSucceeded() {
204-
return config_loading_status_.ok();
205-
}
206-
207186
utils::Status ApiManagerImpl::GetStatistics(
208187
ApiManagerStatistics *statistics) const {
209188
memset(&statistics->service_control_statistics, 0,
@@ -220,15 +199,10 @@ utils::Status ApiManagerImpl::GetStatistics(
220199
return utils::Status::OK;
221200
}
222201

223-
void ApiManagerImpl::AddPendingRequestCallback(
224-
std::function<void(utils::Status)> callback) {
225-
pending_request_callbacks_.push_back(callback);
226-
}
227-
228202
std::unique_ptr<RequestHandlerInterface> ApiManagerImpl::CreateRequestHandler(
229203
std::unique_ptr<Request> request_data) {
230-
return std::unique_ptr<RequestHandlerInterface>(
231-
new RequestHandler(this, check_workflow_, std::move(request_data)));
204+
return std::unique_ptr<RequestHandlerInterface>(new RequestHandler(
205+
check_workflow_, SelectService(), std::move(request_data)));
232206
}
233207

234208
std::shared_ptr<ApiManager> ApiManagerFactory::CreateApiManager(

‎contrib/endpoints/src/api_manager/api_manager_impl.h

Lines changed: 6 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -58,27 +58,17 @@ class ApiManagerImpl : public ApiManager {
5858
utils::Status AddConfig(const std::string &service_config, bool initialize,
5959
std::string *config_id);
6060

61-
// Use these configs according to the traffic percentage.
62-
void DeployConfigs(std::vector<std::pair<std::string, int>> &&list);
63-
64-
// Return the initialization status
65-
inline utils::Status ConfigLoadingStatus() { return config_loading_status_; }
66-
6761
// Return ServiceContext for selected by WeightedSelector
6862
std::shared_ptr<context::ServiceContext> SelectService();
6963

70-
// Append Check or Report callback
71-
void AddPendingRequestCallback(std::function<void(utils::Status)> callback);
72-
73-
// Return true if config loading is still in progress
74-
bool IsConfigLoadingInProgress();
75-
76-
// Return true if config loading was succeeded
77-
bool IsConfigLoadingSucceeded();
64+
// Return the initialization status
65+
inline utils::Status ConfigLoadingStatus() const override {
66+
return config_loading_status_;
67+
}
7868

7969
private:
80-
// Notify pending requests
81-
void NotifyPendingRequests(utils::Status status);
70+
// Use these configs according to the traffic percentage.
71+
void DeployConfigs(std::vector<std::pair<std::string, int>> &&list);
8272

8373
// The check work flow.
8474
std::shared_ptr<CheckWorkflow> check_workflow_;
@@ -103,9 +93,6 @@ class ApiManagerImpl : public ApiManager {
10393
// - Code::OK Successfully initialized
10494
// - Code::ABORTED Initialization was failed
10595
utils::Status config_loading_status_;
106-
107-
// List of pending Check and Report callback
108-
std::vector<std::function<void(utils::Status)>> pending_request_callbacks_;
10996
};
11097

11198
} // namespace api_manager

‎contrib/endpoints/src/api_manager/api_manager_test.cc

Lines changed: 0 additions & 61 deletions
Original file line numberDiff line numberDiff line change
@@ -213,14 +213,8 @@ TEST_F(ApiManagerTest, InitializedByConfigManager) {
213213
EXPECT_EQ("bookstore.test.appspot.com", api_manager->service_name());
214214
EXPECT_EQ("", api_manager->service("2017-05-01r0").id());
215215

216-
EXPECT_TRUE(api_manager->IsConfigLoadingInProgress());
217-
EXPECT_FALSE(api_manager->IsConfigLoadingSucceeded());
218-
219216
api_manager->Init();
220217

221-
EXPECT_FALSE(api_manager->IsConfigLoadingInProgress());
222-
EXPECT_TRUE(api_manager->IsConfigLoadingSucceeded());
223-
224218
EXPECT_EQ("OK", api_manager->ConfigLoadingStatus().ToString());
225219
EXPECT_TRUE(api_manager->Enabled());
226220
EXPECT_EQ("2017-05-01r0", api_manager->service("2017-05-01r0").id());
@@ -250,75 +244,20 @@ TEST_F(ApiManagerTest, ConfigManagerInitializationFailed) {
250244
std::move(env), "", kServerConfigWithServiceNameConfigId)));
251245

252246
EXPECT_TRUE(api_manager);
253-
EXPECT_TRUE(api_manager->IsConfigLoadingInProgress());
254-
EXPECT_FALSE(api_manager->IsConfigLoadingSucceeded());
255-
256247
EXPECT_EQ("UNAVAILABLE: Not initialized yet",
257248
api_manager->ConfigLoadingStatus().ToString());
258249
EXPECT_EQ("bookstore.test.appspot.com", api_manager->service_name());
259250
EXPECT_EQ("", api_manager->service("2017-05-01r0").id());
260251

261252
api_manager->Init();
262253

263-
EXPECT_FALSE(api_manager->IsConfigLoadingInProgress());
264-
EXPECT_FALSE(api_manager->IsConfigLoadingSucceeded());
265-
266254
EXPECT_EQ("ABORTED: Failed to download the service config",
267255
api_manager->ConfigLoadingStatus().ToString());
268256

269257
auto service = api_manager->SelectService();
270258
EXPECT_FALSE(service);
271259
}
272260

273-
TEST_F(ApiManagerTest, AddPendingCallbackThenInitializationSucceeded) {
274-
std::unique_ptr<MockApiManagerEnvironment> env(
275-
new ::testing::NiceMock<MockApiManagerEnvironment>());
276-
277-
EXPECT_CALL(*(env.get()), DoRunHTTPRequest(_))
278-
.WillOnce(Invoke([this](HTTPRequest *req) {
279-
EXPECT_EQ(
280-
"https://servicemanagement.googleapis.com/v1/services/"
281-
"bookstore.test.appspot.com/configs/2017-05-01r0",
282-
req->url());
283-
req->OnComplete(Status::OK, {}, std::move(kServiceConfig1));
284-
}));
285-
286-
std::shared_ptr<ApiManagerImpl> api_manager(
287-
std::dynamic_pointer_cast<ApiManagerImpl>(MakeApiManager(
288-
std::move(env), "", kServerConfigWithServiceNameConfigId)));
289-
290-
EXPECT_TRUE(api_manager);
291-
EXPECT_EQ("UNAVAILABLE: Not initialized yet",
292-
api_manager->ConfigLoadingStatus().ToString());
293-
EXPECT_EQ("bookstore.test.appspot.com", api_manager->service_name());
294-
EXPECT_EQ("", api_manager->service("2017-05-01r0").id());
295-
296-
EXPECT_TRUE(api_manager->IsConfigLoadingInProgress());
297-
EXPECT_FALSE(api_manager->IsConfigLoadingSucceeded());
298-
299-
api_manager->AddPendingRequestCallback([this](utils::Status status) {
300-
callback_run_count_++;
301-
EXPECT_OK(status);
302-
});
303-
EXPECT_EQ(0, callback_run_count_);
304-
305-
api_manager->Init();
306-
307-
EXPECT_EQ(1, callback_run_count_);
308-
309-
EXPECT_FALSE(api_manager->IsConfigLoadingInProgress());
310-
EXPECT_TRUE(api_manager->IsConfigLoadingSucceeded());
311-
312-
EXPECT_OK(api_manager->ConfigLoadingStatus());
313-
EXPECT_TRUE(api_manager->Enabled());
314-
EXPECT_EQ("2017-05-01r0", api_manager->service("2017-05-01r0").id());
315-
316-
auto service = api_manager->SelectService();
317-
EXPECT_TRUE(service);
318-
EXPECT_EQ("bookstore.test.appspot.com", service->service_name());
319-
EXPECT_EQ("2017-05-01r0", service->service().id());
320-
}
321-
322261
TEST_F(ApiManagerTest,
323262
InitializationFailedByConfigManagerWithDifferentServiceName) {
324263
std::unique_ptr<MockApiManagerEnvironment> env(

‎contrib/endpoints/src/api_manager/request_handler.cc

Lines changed: 2 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -30,36 +30,7 @@ namespace google {
3030
namespace api_manager {
3131

3232
void RequestHandler::Check(std::function<void(Status status)> continuation) {
33-
if (api_manager_->IsConfigLoadingInProgress()) {
34-
api_manager_->AddPendingRequestCallback(
35-
[continuation, this](utils::Status status) {
36-
if (status.ok()) {
37-
InternalCheck(continuation);
38-
} else {
39-
// Let esp pass through the client request.
40-
continuation(utils::Status::OK);
41-
}
42-
});
43-
} else if (api_manager_->IsConfigLoadingSucceeded()) {
44-
InternalCheck(continuation);
45-
} else {
46-
// Let esp pass through the client request.
47-
continuation(utils::Status::OK);
48-
}
49-
}
50-
51-
void RequestHandler::InternalCheck(
52-
std::function<void(utils::Status status)> continuation) {
53-
if (!context_) {
54-
if (!CreateRequestContext()) {
55-
context_->service_context()->env()->LogError(
56-
"Failed to create a request context");
57-
continuation(utils::Status::OK);
58-
return;
59-
}
60-
}
61-
62-
auto interception = [continuation, this](utils::Status status) {
33+
auto interception = [continuation, this](Status status) {
6334
if (status.ok() && context_->cloud_trace()) {
6435
context_->StartBackendSpanAndSetTraceContext();
6536
}
@@ -108,34 +79,6 @@ void RequestHandler::AttemptIntermediateReport() {
10879
// Sends a report.
10980
void RequestHandler::Report(std::unique_ptr<Response> response,
11081
std::function<void(void)> continuation) {
111-
if (api_manager_->IsConfigLoadingInProgress()) {
112-
Response* response_ptr = response.release();
113-
api_manager_->AddPendingRequestCallback([response_ptr, continuation,
114-
this](utils::Status status) {
115-
if (status.ok()) {
116-
InternalReport(std::unique_ptr<Response>(response_ptr), continuation);
117-
} else {
118-
continuation();
119-
}
120-
});
121-
} else if (api_manager_->IsConfigLoadingSucceeded()) {
122-
InternalReport(std::move(response), continuation);
123-
} else {
124-
continuation();
125-
}
126-
}
127-
128-
void RequestHandler::InternalReport(std::unique_ptr<Response> response,
129-
std::function<void(void)> continuation) {
130-
if (!context_) {
131-
if (!CreateRequestContext()) {
132-
context_->service_context()->env()->LogError(
133-
"Failed to create a request context");
134-
continuation();
135-
return;
136-
}
137-
}
138-
13982
// Close backend trace span.
14083
context_->EndBackendSpan();
14184

@@ -173,7 +116,7 @@ void RequestHandler::InternalReport(std::unique_ptr<Response> response,
173116
}
174117

175118
std::string RequestHandler::GetServiceConfigId() const {
176-
return context_ ? context_->service_context()->service().id() : "";
119+
return context_->service_context()->service().id();
177120
}
178121

179122
std::string RequestHandler::GetBackendAddress() const {
@@ -193,15 +136,5 @@ std::string RequestHandler::GetRpcMethodFullName() const {
193136
}
194137
}
195138

196-
bool RequestHandler::CreateRequestContext() {
197-
auto service_context = api_manager_->SelectService();
198-
if (service_context) {
199-
context_ = std::make_shared<context::RequestContext>(
200-
service_context, std::move(request_data_));
201-
}
202-
203-
return context_ != nullptr;
204-
}
205-
206139
} // namespace api_manager
207140
} // namespace google

‎contrib/endpoints/src/api_manager/request_handler.h

Lines changed: 7 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -26,12 +26,12 @@ namespace api_manager {
2626
// Implements RequestHandlerInterface.
2727
class RequestHandler : public RequestHandlerInterface {
2828
public:
29-
RequestHandler(ApiManagerImpl* api_manager,
30-
std::shared_ptr<CheckWorkflow> check_workflow,
29+
RequestHandler(std::shared_ptr<CheckWorkflow> check_workflow,
30+
std::shared_ptr<context::ServiceContext> service_context,
3131
std::unique_ptr<Request> request_data)
32-
: api_manager_(api_manager),
33-
check_workflow_(check_workflow),
34-
request_data_(std::move(request_data)) {}
32+
: context_(new context::RequestContext(service_context,
33+
std::move(request_data))),
34+
check_workflow_(check_workflow) {}
3535

3636
virtual ~RequestHandler(){};
3737

@@ -49,32 +49,19 @@ class RequestHandler : public RequestHandlerInterface {
4949
virtual std::string GetRpcMethodFullName() const;
5050

5151
// Get the method info.
52-
const MethodInfo* method() const { return context_->method(); }
52+
const MethodInfo *method() const { return context_->method(); }
5353

5454
// Get the method info.
55-
const MethodCallInfo* method_call() const { return context_->method_call(); }
55+
const MethodCallInfo *method_call() const { return context_->method_call(); }
5656

5757
private:
58-
// Create a context_ from ApiManager. Return true if the RequestContext was
59-
// successfully created
60-
bool CreateRequestContext();
61-
// Internal Check
62-
void InternalCheck(std::function<void(utils::Status status)> continuation);
63-
// Internal Report
64-
void InternalReport(std::unique_ptr<Response> response,
65-
std::function<void(void)> continuation);
66-
// ApiManager instance
67-
ApiManagerImpl* api_manager_;
68-
6958
// The context object needs to pass to the continuation function the check
7059
// handler as a lambda capture so it can be passed to the next check handler.
7160
// In order to control the life time of context object, a shared_ptr is used.
7261
// This object holds a ref_count, the continuation will hold another one.
7362
std::shared_ptr<context::RequestContext> context_;
7463

7564
std::shared_ptr<CheckWorkflow> check_workflow_;
76-
// Unique copy of the request data to initialize context_ later
77-
std::unique_ptr<Request> request_data_;
7865
};
7966

8067
} // namespace api_manager

0 commit comments

Comments
 (0)