> 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.
> `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?) - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66460/#review200842 ----------------------------------------------------------- On April 6, 2018, 9:17 p.m., Gaston Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66460/ > ----------------------------------------------------------- > > (Updated April 6, 2018, 9:17 p.m.) > > > Review request for mesos, Greg Mann and Vinod Kone. > > > 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 > >
