Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review201763
---


Ship it!




Ship It!

- Greg Mann


On April 23, 2018, 8:23 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66460/
> ---
> 
> (Updated April 23, 2018, 8:23 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 
>   include/mesos/v1/scheduler/scheduler.proto 
> 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> 60b17b9be74132c81532d22eba681feb899b22a3 
>   src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 
> 
> 
> Diff: https://reviews.apache.org/r/66460/diff/5/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/
---

(Updated April 23, 2018, 1:23 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

Addressed feedback.


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 (updated)
-

  include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 
  include/mesos/v1/scheduler/scheduler.proto 
25ebcfcb07cb4fb928dcfdc242bb6509c9080491 
  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
60b17b9be74132c81532d22eba681feb899b22a3 
  src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 


Diff: https://reviews.apache.org/r/66460/diff/5/

Changes: https://reviews.apache.org/r/66460/diff/4-5/


Testing
---

`sudo bin/mesos-tests` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review201682
---




include/mesos/v1/scheduler/scheduler.proto
Lines 500 (patched)


s/ty/by/

I'll fix this while committing.



include/mesos/v1/scheduler/scheduler.proto
Lines 516 (patched)


s/not part/is not part/

I'll fix this while committing.


- Greg Mann


On April 21, 2018, 12:03 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66460/
> ---
> 
> (Updated April 21, 2018, 12:03 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 
>   include/mesos/v1/scheduler/scheduler.proto 
> 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> 60b17b9be74132c81532d22eba681feb899b22a3 
>   src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 
> 
> 
> Diff: https://reviews.apache.org/r/66460/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review201673
---



Changed the return type to the following proto message:

```
/**
 * This message is used ty the C++ Scheduler HTTP API library as the return
 * type of the `call()` method. The message includes the HTTP status code with
 * which the master responded, and optionally a `scheduler::Response` message.
 *
 * There are three cases to consider depending on the HTTP response status code:
 *
 *  (1) '202 ACCEPTED': Indicates the call was accepted for processing and
 *neither `response` nor `error` will be set.
 *
 *  (2) '200 OK': Indicates the call completed successfully, and the `response`
 *field will be set if the `scheduler::Call::Type` has a corresponding
 *`scheduler::Response::Type`; `error` will not be set.
 *
 *  (3) For all other HTTP status codes, the `response` field will not be set
 *  and the `error` field may be set to provide more information.
 *
 * NOTE: This message is used by the C++ Scheduler HTTP API library and not part
 * of the API specification.
 */
message APIResult {
  // HTTP status code with which the master responded.
  required uint32 status_code = 1;

  // This field will only be set if the call completed successfully and the
  // master responded with `200 OK` and a non-empty body.
  optional Response response = 2;

  // This field will only be set if the call did not complete successfully and
  // the master responded with a status other than `202 Accepted` or `200 OK`,
  // and with a non-empty body.
  optional string error = 3;
}
```

I tried to reply to BenM's issue, but Review Board doesn't let me publish the 
draft ¯_(?)_/¯.

- Gaston Kleiman


On April 20, 2018, 5:03 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66460/
> ---
> 
> (Updated April 20, 2018, 5:03 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 
>   include/mesos/v1/scheduler/scheduler.proto 
> 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> 60b17b9be74132c81532d22eba681feb899b22a3 
>   src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 
> 
> 
> Diff: https://reviews.apache.org/r/66460/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

> On April 6, 2018, 2:13 p.m., Vinod Kone wrote:
> > include/mesos/v1/scheduler.hpp
> > Line 50 (original), 54-55 (patched)
> > 
> >
> > Since we don't guarantee backwards compat for this library, can we just 
> > update the signature of `send`?
> 
> Gaston Kleiman wrote:
> Would the libmesos binary then be compatible? Or would frameworks then 
> have to be recompiled using this new header in order to use new libmesos 
> binaries?
> 
> Greg Mann wrote:
> So, it sounds like if we broke the existing scheduler library interface, 
> that would mean that Java frameworks using the V0->V1 adapter would need to 
> simultaneously upgrade their Mesos JAR and libmesos. This is probably fine, 
> but we're checking with some framework developers to confirm.

Updating the signature of `send` would break people who have installed 
schedulers from packages that don't bundle the Mesos JAR and libmesos together, 
e.g., people who installed Marathon/Chronos and Mesos from rpm or deb packages.


- Gaston


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200670
---


On April 20, 2018, 5:03 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66460/
> ---
> 
> (Updated April 20, 2018, 5:03 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 
>   include/mesos/v1/scheduler/scheduler.proto 
> 25ebcfcb07cb4fb928dcfdc242bb6509c9080491 
>   src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
> 60b17b9be74132c81532d22eba681feb899b22a3 
>   src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 
> 
> 
> Diff: https://reviews.apache.org/r/66460/diff/4/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/
---

(Updated April 20, 2018, 5:03 p.m.)


Review request for mesos, Greg Mann and Vinod Kone.


Changes
---

Changed the signature of the new method.


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 (updated)
-

  include/mesos/v1/scheduler.hpp d56e08861d5190ef66992d383dc5710d8f6ce661 
  include/mesos/v1/scheduler/scheduler.proto 
25ebcfcb07cb4fb928dcfdc242bb6509c9080491 
  src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp 
60b17b9be74132c81532d22eba681feb899b22a3 
  src/scheduler/scheduler.cpp ecef916ffd0797a13552525ff2cda0f99ca57e74 


Diff: https://reviews.apache.org/r/66460/diff/4/

Changes: https://reviews.apache.org/r/66460/diff/3-4/


Testing
---

`sudo bin/mesos-tests` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Benjamin Mahler]

> On April 10, 2018, 8:45 p.m., Benjamin Mahler wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 102-114 (patched)
> > 
> >
> > Hm.. why does this return an `Option`?
> > 
> > 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 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>>`.
> 
> 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`.
> 
> 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>>`
> 
> 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>>`
> 
> > 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 

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

> On April 10, 2018, 1:45 p.m., Benjamin Mahler wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 102-114 (patched)
> > 
> >
> > Hm.. why does this return an `Option`?
> > 
> > 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 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>>`.
> 
> 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`.
> 
> 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>>`
> 
> 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>>`

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

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

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


Changes
---

Updated metadata.


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

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Benjamin Mahler]

> On April 10, 2018, 8:45 p.m., Benjamin Mahler wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 102-114 (patched)
> > 
> >
> > Hm.. why does this return an `Option`?
> > 
> > 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 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>>`.
> 
> 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`.
> 
> 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>>`

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

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Greg Mann]

> On April 10, 2018, 8:45 p.m., Benjamin Mahler wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 102-114 (patched)
> > 
> >
> > Hm.. why does this return an `Option`?
> > 
> > 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 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>>`.
> 
> 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`.
> 
> 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
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

> On April 10, 2018, 1:45 p.m., Benjamin Mahler wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 102-114 (patched)
> > 
> >
> > Hm.. why does this return an `Option`?
> > 
> > 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 master::Response>>` where master::Response is returned for 200 OK? (If we 
> want to handle parsing into master::Response for them.

Most scheduler API calls don't return a `scheduler::Response`, so we'd need to 
use `Future>>`.

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

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.


- Gaston


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200842
---


On April 6, 2018, 2: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, 2: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
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Benjamin Mahler]

> On April 10, 2018, 8:45 p.m., Benjamin Mahler wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 102-114 (patched)
> > 
> >
> > Hm.. why does this return an `Option`?
> > 
> > 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.

How do I program against that? Parse the failure.message() into an 
http::Response?

It looks like we need: `Future>` 
where master::Response is returned for 200 OK? (If we want to handle parsing 
into master::Response for them.


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

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

> On April 10, 2018, 1:45 p.m., Benjamin Mahler wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 102-114 (patched)
> > 
> >
> > Hm.. why does this return an `Option`?
> > 
> > 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).

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.


- Gaston


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200842
---


On April 6, 2018, 2: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, 2: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
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Benjamin Mahler]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200842
---




include/mesos/v1/scheduler.hpp
Lines 102-114 (patched)


Hm.. why does this return an `Option`?

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


- Benjamin Mahler


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

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Greg Mann]

> On April 6, 2018, 9:13 p.m., Vinod Kone wrote:
> > include/mesos/v1/scheduler.hpp
> > Line 50 (original), 54-55 (patched)
> > 
> >
> > Since we don't guarantee backwards compat for this library, can we just 
> > update the signature of `send`?
> 
> Gaston Kleiman wrote:
> Would the libmesos binary then be compatible? Or would frameworks then 
> have to be recompiled using this new header in order to use new libmesos 
> binaries?

So, it sounds like if we broke the existing scheduler library interface, that 
would mean that Java frameworks using the V0->V1 adapter would need to 
simultaneously upgrade their Mesos JAR and libmesos. This is probably fine, but 
we're checking with some framework developers to confirm.


- Greg


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200670
---


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

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/
---

(Updated April 6, 2018, 2:17 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Fixed typo.


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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/66460/diff/2-3/


Testing
---

`sudo bin/mesos-tests` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

> On April 6, 2018, 2:13 p.m., Vinod Kone wrote:
> > include/mesos/v1/scheduler.hpp
> > Line 50 (original), 54-55 (patched)
> > 
> >
> > Since we don't guarantee backwards compat for this library, can we just 
> > update the signature of `send`?

Would the libmesos binary then be compatible? Or would frameworks then have to 
be recompiled using this new header in order to use new libmesos binaries?


> On April 6, 2018, 2:13 p.m., Vinod Kone wrote:
> > include/mesos/v1/scheduler.hpp
> > Lines 112-113 (patched)
> > 
> >
> > What do you mean by `SUBSCRIBED` calls?

There's is a typo there, I mean `SUBSCRIBE` calls, for which the response is a 
stream.


- Gaston


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200670
---


On April 6, 2018, 2 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, 2 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/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200670
---




include/mesos/v1/scheduler.hpp
Line 50 (original), 54-55 (patched)


Since we don't guarantee backwards compat for this library, can we just 
update the signature of `send`?



include/mesos/v1/scheduler.hpp
Lines 112-113 (patched)


What do you mean by `SUBSCRIBED` calls?


- Vinod Kone


On April 6, 2018, 9 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 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/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/
---

(Updated April 6, 2018, 1:59 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Addressed Greg's feedback.


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 (updated)
-

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

Changes: https://reviews.apache.org/r/66460/diff/1-2/


Testing
---

`sudo bin/mesos-tests` on GNU/Linux


Thanks,

Gaston Kleiman

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

> On April 6, 2018, 1:24 p.m., Greg Mann wrote:
> > src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
> > Lines 143 (patched)
> > 
> >
> > What do you think about using `LOG(FATAL)` here instead of 
> > `UNREACHABLE()` in order to provide some useful logging before terminating? 
> > Might be nicer for a user than having to dig through the source to 
> > understand exactly what's going on.

This is consistent with what we do for `reconnect()`, right above this method.

Note that there are no JNI bindings for this method, so it should really be 
impossible to reach it =).


> On April 6, 2018, 1:24 p.m., Greg Mann wrote:
> > src/scheduler/scheduler.cpp
> > Lines 266 (patched)
> > 
> >
> > Suggestion: I think it might improve readability slightly to use a name 
> > like `callMessage` instead of `call_`? Here and in the continuations below. 
> > Up to you.

Good idea! Me likey!


- Gaston


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200661
---


On April 4, 2018, 4:53 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66460/
> ---
> 
> (Updated April 4, 2018, 4:53 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> 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/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Greg Mann]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/#review200661
---




src/java/jni/org_apache_mesos_v1_scheduler_V0Mesos.cpp
Lines 143 (patched)


What do you think about using `LOG(FATAL)` here instead of `UNREACHABLE()` 
in order to provide some useful logging before terminating? Might be nicer for 
a user than having to dig through the source to understand exactly what's going 
on.



src/scheduler/scheduler.cpp
Lines 266 (patched)


Suggestion: I think it might improve readability slightly to use a name 
like `callMessage` instead of `call_`? Here and in the continuations below. Up 
to you.



src/scheduler/scheduler.cpp
Lines 760-762 (patched)


Hmm is this an invariant we want to bake into this library? Another option 
might be:

```
LOG(WARNING) << "Response for " << call_.type() << " unexpectedly included 
body: '" << response.body << "'";
```



src/scheduler/scheduler.cpp
Lines 783-785 (patched)


Is this comment accurate? If the response body is non-empty and couldn't be 
deserialized, then we would hit the `.isError()` case above, right?


- Greg Mann


On April 4, 2018, 11:53 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66460/
> ---
> 
> (Updated April 4, 2018, 11:53 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> 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/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Review Request 66460: Added a `call()` method to the v1 scheduler library.

[Gaston Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/66460/
---

Review request for mesos and Greg Mann.


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


Testing
---

`sudo bin/mesos-tests` on GNU/Linux


Thanks,

Gaston Kleiman