> On July 29, 2015, 7:12 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 2128-2159
> > <https://reviews.apache.org/r/36720/diff/3/?file=1021871#file1021871line2128>
> >
> > Any reason you're skipping validation (authorization) of the framework? Looks
like we should pull out the pid specific code from validate for now: it looks like we already
have to do synchronous checks inside the continuations anyway. For example, `_reregisterFramework`
checks to see if it is authenticated, but it is missing the check that it is the right principal!
> >
> > Looks like we should s/validate/authorize/ and pull out an `isAuthenticated`
that we can call synchronously as well.
Primarily due to the reasons you had mentioned earlier. There was a lot of pid specific code
in validate. Also , call validation in general were supposed to be handled in MESOS-2497,
so I refrained from making the changes. Looks like you already did them in r36919, Thanks
!
> On July 29, 2015, 7:12 p.m., Ben Mahler wrote:
> > src/master/master.cpp, lines 2162-2193
> > <https://reviews.apache.org/r/36720/diff/3/?file=1021871#file1021871line2162>
> >
> > This was a bit confusing, since it says "registering" but you're planning to
also have it include re-registration? This looks like `_subscribe` given it is the continuation.
Yes, I intend to make re-register also call into this function. You are also right about this
being the "_subscribe".
Since I had not yet taken care of validation as per my earlier argument, I went ahead with
naming it as subscribe for now. When we have the validation routine , it would just become
a continuation function and can be renamed. Also , failover logic would be implemented inside
this function and the existing _reregisterFramework(...)
would be calling into this function. What do you think ?
> On July 29, 2015, 7:12 p.m., Ben Mahler wrote:
> > src/master/http.cpp, lines 324-379
> > <https://reviews.apache.org/r/36720/diff/3/?file=1021869#file1021869line324>
> >
> > JSON should be do-able already, here's a suggestion to clean this up a bit (note
that it is handling JSON):
> >
> > ```
> > Future<Response> Master::Http::call(const Request& request) const
> > {
> > scheduler::Call call;
> >
> > // TODO(anand): Content type values are case-insensitive.
> > Option<string> contentType = request.headers.get("Content-Type");
> >
> > if (contentType.isNone()) {
> > return BadRequest("Expecting 'Content-Type' to be present");
> > }
> >
> > if (contentType.get() == APPLICATION_PROTOBUF) {
> > if (!call.ParseFromString(request.body)) {
> > return BadRequest("Failed to deserialize body into Call protobuf");
> > }
> > } else if (contentType.get() == APPLICATION_JSON) {
> > Try<JSON::Value> value = JSON::parse(request.body);
> >
> > if (value.isError()) {
> > return BadRequest("Failed to parse body into JSON: " + value.error());
> > }
> >
> > Try<scheduler::Call> parse =
> > ::protobuf::parse<scheduler::Call>(value.get());
> >
> > if (parse.isError()) {
> > return BadRequest("Failed to convert JSON into Call protobuf: " +
> > parse.error());
> > }
> >
> > call = parse.get();
> > }
> >
> > // Default to sending back JSON.
> > ContentType responseContentType = ContentType::JSON;
> >
> > // TODO(anand): Use request.accepts() once available.
> > Option<string> acceptType = request.headers.get("Accept");
> >
> > if (acceptType.get() == APPLICATION_PROTOBUF) {
> > responseContentType = ContentType::PROTOBUF;
> > }
> >
> > switch (call.type()) {
> > case scheduler::Call::SUBSCRIBE: {
> > Pipe pipe;
> > OK ok;
> >
> > ok.type = Response::PIPE;
> > ok.reader = pipe.reader();
> >
> > master->subscribeHttp(
> > call.subscribe(),
> > responseContentType,
> > pipe.writer());
> >
> > return ok;
> > }
> > default:
> > break;
> > }
> >
> > return NotImplemented();
> > }
> > ```
Thanks, I would include the JSON logic. I thought adding the JSON workflow might be more convoluted
then this and hence wanted it to be part of a different patch, my bad.
- Anand
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/36720/#review93499
-----------------------------------------------------------
On July 25, 2015, 2:32 p.m., Anand Mazumdar wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36720/
> -----------------------------------------------------------
>
> (Updated July 25, 2015, 2:32 p.m.)
>
>
> Review request for mesos and Ben Mahler.
>
>
> Bugs: MESOS-2294
> https://issues.apache.org/jira/browse/MESOS-2294
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Split review out of r36318. This change adds the functionality of making a http call
for subscribe and the master responding with a subscribed event on the persistent stream.
>
>
> Diffs
> -----
>
> src/master/http.cpp 3a1598fad4db03e5f62fd4a6bd26b2bedeee4070
> src/master/master.hpp 827d0d599912b2936beb9615610f627f6c9a2d43
> src/master/master.cpp 613a011e205611702921179b6c436d62447e2dca
> src/tests/http_api_tests.cpp 64bbeb6699171e85a5be293919ad9f32ded0ebac
>
> Diff: https://reviews.apache.org/r/36720/diff/
>
>
> Testing
> -------
>
> make check + test.
>
>
> Thanks,
>
> Anand Mazumdar
>
>
|