> On April 10, 2018, 8: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?) > > Gaston Kleiman 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? > > 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.
> I also find checking whether an option is set or not less awkward and more > readable than checking whether scheduler::Response::type is set. Oh, I was being unclear and didn't intend to implying that was a good thing to do :) (i.e. it would be more consistent to have a Response::type for each Call::type). I think it's easier to reason about always having a response for '200 OK', as this is generally how request/response RPC works. Notice how grpc doesn't have the notion of no response (unless I'm missing something): https://grpc.io/docs/guides/concepts.html#service-definition '202 Accepted' is probably where it becomes easier to reason about a lack of response (we only accepted your message). I imagine this is the case we would do away with if we move to something like grpc. Is it the case that all of the None cases are 202's? > I think that ideally we'd like "abstract" response codes such as (or maybe > exactly) the ones used by grpc. Sounds good. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200842 ----------------------------------------------------------- On April 11, 2018, 2:25 a.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > ----------------------------------------------------------- > > (Updated April 11, 2018, 2:25 a.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 > >
