> 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 > >
