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

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.


- Greg


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

Reply via email to