> On April 10, 2018, 1:45 p.m., Benjamin Mahler wrote: > > include/mesos/v1/scheduler.hpp > > Lines 102-114 (patched) > > <https://reviews.apache.org/r/66460/diff/3/?file=1993428#file1993428line102> > > > > Hm.. why does this return an `Option<Response>`? > > > > If this is mesos Response rather than http::Response, aren't we losing > > information about which http code came back? (e.g. 400, 401, etc). > > Gaston Kleiman wrote: > If the request is not successful and the http code is neither 200 nor > 202, the method will return an error that includes the http code and the body > of the HTTP response. > > It returns a `scheduler::Response` in order to abstract the consumer from > the transport layer and prevent it from having to understand and deserialize > HTTP responses. > > Benjamin Mahler wrote: > How do I program against that? Parse the failure.message() into an > http::Response? > > It looks like we need: `Future<variant<http::Response, > master::Response>>` where master::Response is returned for 200 OK? (If we > want to handle parsing into master::Response for them. > > Gaston Kleiman wrote: > Most scheduler API calls don't return a `scheduler::Response`, so we'd > need to use `Future<variant<http::Response, Option<scheduler::Response>>>`. > > However that would only give the consumer access to the `http::Response` > if the request fails. We might want to return an enum that always contains > the `http::Response` and that also includes an `Option<scheduler::Response>`. > > Note that the current call `send()` is a `void` method, so we've been > able to avoid using the `http::Response` sent by the master. > > Greg Mann wrote: > This library does not expose the transport layer to the client at all, > and the current patch maintains that convention. > > If we're going to expose the HTTP response, I think Gastón's suggestion > of including it unconditionally makes sense. > > Without the HTTP response, yes I think the client would have to parse the > failure message to deduce the error. Perhaps this is a suitable time to start > exposing HTTP here? I would be fine with that. > > Benjamin Mahler wrote: > > `Future<variant<http::Response, Option<scheduler::Response>>>` > > Are you sure you need the option? I think the client can always decode a > scheduler::Response, because when the master sends nothing back, the client > can successfully decode an empty `scheduler::Response`, it just won't have > `Response::type` set? In any case, the inconsistency here seems a bit > confusing? > > > This library does not expose the transport layer to the client at all, > and the current patch maintains that convention. > > HTTP is in the application layer rather than tranport layer (to be > pendantic :)), and that makes sense here since it encodes some of the > application level responses from the master rather than all application > behavior being inside `master::Response`. E.g. 400 Bad Request > > > Perhaps this is a suitable time to start exposing HTTP here? I would be > fine with that. > > It's either that, or we "abstract" response codes / bodies out into some > other structure, but it doesn't seem maintainable as more http response codes > get returned by the master? > > Anyway, sorry to throw a wrench in here :) I just want to avoid parsing > strings to program against the master. (I'm assuming there will be some > response codes that the scheduler will react differently to: e.g. 400 > shouldn't be retried, whereas 503 should?)
> > `Future<variant<http::Response, Option<scheduler::Response>>>` > Are you sure you need the option? I think the client can always decode a > scheduler::Response, because when the master sends nothing back, the client > can successfully decode an empty scheduler::Response, it just won't have > Response::type set? In any case, the inconsistency here seems a bit confusing? I think that using an option makes it semantically clearer that the master might not respond to a scheduler API call with a `scheduler::Response`. I also find checking whether an option is set or not less awkward and more readable than checking whether `scheduler::Response::type` is set. An HTTP will always be returned if the future didn't fail, but a `scheduler::Response` is optional and the master in most cases doesn't respond with one. I don't think that we need symmetry/consistency in the method signature. > > Perhaps this is a suitable time to start exposing HTTP here? I would be > > fine with that. > It's either that, or we "abstract" response codes / bodies out into some > other structure, but it doesn't seem maintainable as more http response codes > get returned by the master? I think that ideally we'd like "abstract" response codes such as (or maybe exactly) [the ones used by grpc](https://github.com/grpc/grpc/blob/master/doc/statuscodes.md). This would for instance help disambiguate 404 responses that can mean that a resource was not found or that the master doesn't know how to handle a route yet. In my opinion this is something worth discussing, but out of the scope of this patch and of MESOS-8192. Greg, Vinod, and I had a meeting, and came up with this possible return type: ```cpp struct Response { struct Status { process::http::Status code; Option<string> errorMessage; } status; Option<scheduler::Response> response; } ``` It is not final and we should have another meeting to discuss it and the name of the method. Vinod also mentioned that since there is no commonly used Java implementation of the API, we probably want to add support for Request/Response API calls to the [Java V1Mesos class](https://github.com/apache/mesos/blob/df89829de5f6accffe92c74476cce319307422c2/src/java/src/org/apache/mesos/v1/scheduler/V1Mesos.java). This would probably involve mapping libprocess/stout `Future` and `Option` classes to Java equivalents, and probably deserves its own ticket. - Gaston ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200842 ----------------------------------------------------------- On April 10, 2018, 7:25 p.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > ----------------------------------------------------------- > > (Updated April 10, 2018, 7:25 p.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > Bugs: MESOS-8192 > https://issues.apache.org/jira/browse/MESOS-8192 > > > Repository: mesos > > > Description > ------- > > This patch adds a `call()` method to the scheduler library that allows > clients to send a `v1::scheduler::Call` to the master and receive a > `v1::scheduler::Response`. > > It is going to be used to test operation state reconciliation. > > > Diffs > ----- > > include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp > 60b17b9be74132c81532d22eba681feb899b22a3 > src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 > > > Diff: https://reviews.apache.org/r/66460/diff/3/ > > > Testing > ------- > > `sudo bin/mesos-tests` on GNU/Linux > > > Thanks, > > Gaston Kleiman > >
