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

Reply via email to