You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@mesos.apache.org by gr...@apache.org on 2019/01/10 18:35:59 UTC
[mesos] 01/08: Narrowed interface of `ReadOnlyHandler` members.
This is an automated email from the ASF dual-hosted git repository.
grag pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 148b7483234b1989518d191b4db4365a1852ca4f
Author: Benno Evers <be...@mesosphere.com>
AuthorDate: Wed Jan 9 14:29:41 2019 -0800
Narrowed interface of `ReadOnlyHandler` members.
Previously, the members of ReadOnlyHandler would take a full
Request as parameter, making it hard for clients to reason
about which parts of the request are used internally, and
even harder to guarantee that behaviour into the future.
This commit changes the interface so only the query
parameters get passed.
Review: https://reviews.apache.org/r/69071/
---
src/master/http.cpp | 30 +++++++++++++++++++-----------
src/master/master.hpp | 22 +++++++++++++---------
src/master/readonly_handler.cpp | 38 +++++++++++++++++++-------------------
3 files changed, 51 insertions(+), 39 deletions(-)
diff --git a/src/master/http.cpp b/src/master/http.cpp
index e38513b..e352ec8 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -1261,7 +1261,9 @@ Future<Response> Master::Http::frameworks(
master->self(),
[this, request](const Owned<ObjectApprovers>& approvers) {
return deferBatchedRequest(
- &Master::ReadOnlyHandler::frameworks, request, approvers);
+ &Master::ReadOnlyHandler::frameworks,
+ request.url.query,
+ approvers);
}));
}
@@ -2061,7 +2063,9 @@ Future<Response> Master::Http::slaves(
master->self(),
[this, request](const Owned<ObjectApprovers>& approvers) {
return deferBatchedRequest(
- &Master::ReadOnlyHandler::slaves, request, approvers);
+ &Master::ReadOnlyHandler::slaves,
+ request.url.query,
+ approvers);
}));
}
@@ -2368,7 +2372,7 @@ Future<Response> Master::Http::state(
[this, request](const Owned<ObjectApprovers>& approvers) {
return deferBatchedRequest(
&Master::ReadOnlyHandler::state,
- request,
+ request.url.query,
approvers);
}));
}
@@ -2376,7 +2380,7 @@ Future<Response> Master::Http::state(
Future<Response> Master::Http::deferBatchedRequest(
ReadOnlyRequestHandler handler,
- const Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const Owned<ObjectApprovers>& approvers) const
{
bool scheduleBatch = batchedRequests.empty();
@@ -2385,7 +2389,7 @@ Future<Response> Master::Http::deferBatchedRequest(
Promise<Response> promise;
Future<Response> future = promise.future();
batchedRequests.push_back(
- BatchedRequest{handler, request, approvers, std::move(promise)});
+ BatchedRequest{handler, queryParameters, approvers, std::move(promise)});
// Schedule processing of batched requests if not yet scheduled.
if (scheduleBatch) {
@@ -2413,12 +2417,12 @@ void Master::Http::processRequestsBatch() const
foreach (BatchedRequest& request, batchedRequests) {
request.promise.associate(process::async(
[this](ReadOnlyRequestHandler handler,
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) {
- return (readonlyHandler.*handler)(request, approvers);
+ return (readonlyHandler.*handler)(queryParameters, approvers);
},
request.handler,
- request.request,
+ request.queryParameters,
request.approvers));
}
@@ -2542,7 +2546,9 @@ Future<Response> Master::Http::stateSummary(
master->self(),
[this, request](const Owned<ObjectApprovers>& approvers) {
return deferBatchedRequest(
- &Master::ReadOnlyHandler::stateSummary, request, approvers);
+ &Master::ReadOnlyHandler::stateSummary,
+ request.url.query,
+ approvers);
}));
}
@@ -2593,7 +2599,7 @@ Future<Response> Master::Http::roles(
[this, request](const Owned<ObjectApprovers>& approvers) {
return deferBatchedRequest(
&Master::ReadOnlyHandler::roles,
- request,
+ request.url.query,
approvers);
}));
}
@@ -2968,7 +2974,9 @@ Future<Response> Master::Http::tasks(
master->self(),
[this, request](const Owned<ObjectApprovers>& approvers) {
return deferBatchedRequest(
- &Master::ReadOnlyHandler::tasks, request, approvers);
+ &Master::ReadOnlyHandler::tasks,
+ request.url.query,
+ approvers);
}));
}
diff --git a/src/master/master.hpp b/src/master/master.hpp
index 99549ab..6230d1d 100644
--- a/src/master/master.hpp
+++ b/src/master/master.hpp
@@ -1290,6 +1290,10 @@ private:
// Inner class used to namespace HTTP handlers that do not change the
// underlying master object.
//
+ // Endpoints served by this handler are only permitted to depend on
+ // the request query parameters and the authorization filters to
+ // make caching of responses possible.
+ //
// NOTE: Most member functions of this class are not routed directly but
// dispatched from their corresponding handlers in the outer `Http` class.
// This is because deciding whether an incoming request is read-only often
@@ -1302,32 +1306,32 @@ private:
// /frameworks
process::http::Response frameworks(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) const;
// /roles
process::http::Response roles(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) const;
// /slaves
process::http::Response slaves(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) const;
// /state
process::http::Response state(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) const;
// /state-summary
process::http::Response stateSummary(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) const;
// /tasks
process::http::Response tasks(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) const;
private:
@@ -1800,12 +1804,12 @@ private:
typedef process::http::Response
(Master::ReadOnlyHandler::*ReadOnlyRequestHandler)(
- const process::http::Request&,
+ const hashmap<std::string, std::string>&,
const process::Owned<ObjectApprovers>&) const;
process::Future<process::http::Response> deferBatchedRequest(
ReadOnlyRequestHandler handler,
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& queryParameters,
const process::Owned<ObjectApprovers>& approvers) const;
void processRequestsBatch() const;
@@ -1813,7 +1817,7 @@ private:
struct BatchedRequest
{
ReadOnlyRequestHandler handler;
- process::http::Request request;
+ hashmap<std::string, std::string> queryParameters;
process::Owned<ObjectApprovers> approvers;
process::Promise<process::http::Response> promise;
};
diff --git a/src/master/readonly_handler.cpp b/src/master/readonly_handler.cpp
index 8895374..66d6160 100644
--- a/src/master/readonly_handler.cpp
+++ b/src/master/readonly_handler.cpp
@@ -637,11 +637,11 @@ private:
process::http::Response Master::ReadOnlyHandler::frameworks(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& query,
const process::Owned<ObjectApprovers>& approvers) const
{
IDAcceptor<FrameworkID> selectFrameworkId(
- request.url.query.get("framework_id"));
+ query.get("framework_id"));
// This lambda is consumed before the outer lambda
// returns, hence capture by reference is fine here.
@@ -690,7 +690,7 @@ process::http::Response Master::ReadOnlyHandler::frameworks(
writer->field("unregistered_frameworks", [](JSON::ArrayWriter*) {});
};
- return OK(jsonify(frameworks), request.url.query.get("jsonp"));
+ return OK(jsonify(frameworks), query.get("jsonp"));
}
@@ -738,7 +738,7 @@ JSON::Object model(
process::http::Response Master::ReadOnlyHandler::roles(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& query,
const process::Owned<ObjectApprovers>& approvers) const
{
JSON::Object object;
@@ -769,24 +769,24 @@ process::http::Response Master::ReadOnlyHandler::roles(
object.values["roles"] = std::move(array);
}
- return OK(object, request.url.query.get("jsonp"));
+ return OK(object, query.get("jsonp"));
}
process::http::Response Master::ReadOnlyHandler::slaves(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& query,
const process::Owned<ObjectApprovers>& approvers) const
{
- IDAcceptor<SlaveID> selectSlaveId(request.url.query.get("slave_id"));
+ IDAcceptor<SlaveID> selectSlaveId(query.get("slave_id"));
return process::http::OK(
jsonify(SlavesWriter(master->slaves, approvers, selectSlaveId)),
- request.url.query.get("jsonp"));
+ query.get("jsonp"));
}
process::http::Response Master::ReadOnlyHandler::state(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& query,
const process::Owned<ObjectApprovers>& approvers) const
{
const Master* master = this->master;
@@ -923,12 +923,12 @@ process::http::Response Master::ReadOnlyHandler::state(
writer->field("unregistered_frameworks", [](JSON::ArrayWriter*) {});
};
- return OK(jsonify(calculateState), request.url.query.get("jsonp"));
+ return OK(jsonify(calculateState), query.get("jsonp"));
}
process::http::Response Master::ReadOnlyHandler::stateSummary(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& query,
const process::Owned<ObjectApprovers>& approvers) const
{
const Master* master = this->master;
@@ -1070,7 +1070,7 @@ process::http::Response Master::ReadOnlyHandler::stateSummary(
});
};
- return OK(jsonify(stateSummary), request.url.query.get("jsonp"));
+ return OK(jsonify(stateSummary), query.get("jsonp"));
}
@@ -1119,21 +1119,21 @@ struct TaskComparator
process::http::Response Master::ReadOnlyHandler::tasks(
- const process::http::Request& request,
+ const hashmap<std::string, std::string>& query,
const process::Owned<ObjectApprovers>& approvers) const
{
// Get list options (limit and offset).
- Result<int> result = numify<int>(request.url.query.get("limit"));
+ Result<int> result = numify<int>(query.get("limit"));
size_t limit = result.isSome() ? result.get() : TASK_LIMIT;
- result = numify<int>(request.url.query.get("offset"));
+ result = numify<int>(query.get("offset"));
size_t offset = result.isSome() ? result.get() : 0;
- Option<string> order = request.url.query.get("order");
+ Option<string> order = query.get("order");
string _order = order.isSome() && (order.get() == "asc") ? "asc" : "des";
- Option<string> frameworkId = request.url.query.get("framework_id");
- Option<string> taskId = request.url.query.get("task_id");
+ Option<string> frameworkId = query.get("framework_id");
+ Option<string> taskId = query.get("task_id");
IDAcceptor<FrameworkID> selectFrameworkId(frameworkId);
IDAcceptor<TaskID> selectTaskId(taskId);
@@ -1223,7 +1223,7 @@ process::http::Response Master::ReadOnlyHandler::tasks(
});
};
- return OK(jsonify(tasksWriter), request.url.query.get("jsonp"));
+ return OK(jsonify(tasksWriter), query.get("jsonp"));
}
} // namespace master {