Re: Review Request 66457: WIP: Made resource provider API aware of workloads.

[Mesos Reviewbot]

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



Bad patch!

Reviews applied: [66457, 66418, 66411, 66410, 66409, 66408, 66407, 66398]

Failed command: python support/apply-reviews.py -n -r 66398

Error:
2018-04-11 04:24:35 URL:https://reviews.apache.org/r/66398/diff/raw/ 
[1889/1889] -> "66398.patch" [1]
error: missing binary patch data for '3rdparty/csi-0.2.0.tar.gz'
error: binary patch does not apply to '3rdparty/csi-0.2.0.tar.gz'
error: 3rdparty/csi-0.2.0.tar.gz: patch does not apply

Full log: https://builds.apache.org/job/Mesos-Reviewbot/22154/console

- Mesos Reviewbot


On April 11, 2018, 2:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66457/
> ---
> 
> (Updated April 11, 2018, 2:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8760
> https://issues.apache.org/jira/browse/MESOS-8760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces a breaking change to change `PublishResources` to
> `UpdateResourceUsage`, so the resource provider is aware of workloads.
> For local resource providers, a workload is a container, and this call
> is responsible to prepare the resources for each container. For external
> resource providers, a workload is a framework, so this call can bookkeep
> the resource usage for each framework and inform the allocator after a
> failover.
> 
> Note that this call is designed to report ALL resources used by every
> workload on an agent, so it can handle resources without identifiers.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> db7c751bb61fb1ee2421015dcbefc021c3afbdac 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 42bc050ed01a272603a41ab052ed75d799dd76e2 
> 
> 
> Diff: https://reviews.apache.org/r/66457/diff/2/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests will be done later in the 
> chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66541: Added default executor test for agent recovery without metadata.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66538, 66539, 66540, 66541]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 10, 2018, 11:41 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66541/
> ---
> 
> (Updated April 10, 2018, 11:41 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8416
> https://issues.apache.org/jira/browse/MESOS-8416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added default executor test for agent recovery without metadata.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 293dd20d882447401572835bd31e197faf76861b 
> 
> 
> Diff: https://reviews.apache.org/r/66541/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that the test crashes without the fix in r/66539.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66457: WIP: Made resource provider API aware of workloads.

[Mesos Reviewbot Windows]

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



FAIL: Failed to apply the dependent review: 66398.

Failed command: `python.exe .\support\apply-reviews.py -n -r 66398`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66457

Relevant logs:

- 
[apply-review-66398-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66457/logs/apply-review-66398-stdout.log):

```
error: missing binary patch data for '3rdparty/csi-0.2.0.tar.gz'
error: binary patch does not apply to '3rdparty/csi-0.2.0.tar.gz'
error: 3rdparty/csi-0.2.0.tar.gz: patch does not apply
```

- Mesos Reviewbot Windows


On April 11, 2018, 2:38 a.m., Chun-Hung Hsiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66457/
> ---
> 
> (Updated April 11, 2018, 2:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-8760
> https://issues.apache.org/jira/browse/MESOS-8760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch introduces a breaking change to change `PublishResources` to
> `UpdateResourceUsage`, so the resource provider is aware of workloads.
> For local resource providers, a workload is a container, and this call
> is responsible to prepare the resources for each container. For external
> resource providers, a workload is a framework, so this call can bookkeep
> the resource usage for each framework and inform the allocator after a
> failover.
> 
> Note that this call is designed to report ALL resources used by every
> workload on an agent, so it can handle resources without identifiers.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> db7c751bb61fb1ee2421015dcbefc021c3afbdac 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 42bc050ed01a272603a41ab052ed75d799dd76e2 
> 
> 
> Diff: https://reviews.apache.org/r/66457/diff/2/
> 
> 
> Testing
> ---
> 
> This patch cannot be compiled standalone. Tests will be done later in the 
> chain.
> 
> 
> Thanks,
> 
> Chun-Hung Hsiao
> 
>

Re: Review Request 66457: WIP: Made resource provider API aware of workloads.

[Chun-Hung Hsiao]

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

(Updated April 11, 2018, 2:38 a.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Jan Schlicht.


Changes
---

Rename `ApplyResourceUsage` to `UpdateResourceUsage`.


Summary (updated)
-

WIP: Made resource provider API aware of workloads.


Bugs: MESOS-8760
https://issues.apache.org/jira/browse/MESOS-8760


Repository: mesos


Description (updated)
---

This patch introduces a breaking change to change `PublishResources` to
`UpdateResourceUsage`, so the resource provider is aware of workloads.
For local resource providers, a workload is a container, and this call
is responsible to prepare the resources for each container. For external
resource providers, a workload is a framework, so this call can bookkeep
the resource usage for each framework and inform the allocator after a
failover.

Note that this call is designed to report ALL resources used by every
workload on an agent, so it can handle resources without identifiers.


Diffs (updated)
-

  include/mesos/resource_provider/resource_provider.proto 
db7c751bb61fb1ee2421015dcbefc021c3afbdac 
  include/mesos/v1/resource_provider/resource_provider.proto 
42bc050ed01a272603a41ab052ed75d799dd76e2 


Diff: https://reviews.apache.org/r/66457/diff/2/

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


Testing
---

This patch cannot be compiled standalone. Tests will be done later in the chain.


Thanks,

Chun-Hung Hsiao

Re: Review Request 66418: Handled failed publish and unpublish CSI calls properly.

[Chun-Hung Hsiao]

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

(Updated April 11, 2018, 2:37 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Bugs: MESOS-8717
https://issues.apache.org/jira/browse/MESOS-8717


Repository: mesos


Description
---

Previously, when a `ControllerPublishVolume` or a `NodePublishVolume`
fails, SLRP assumes the volume states remain unchanged, which are not
suggested by the CSI spec. This patch handles such failures properly.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
a07620d1c4bf618f669575b3e183bf598da905a6 


Diff: https://reviews.apache.org/r/66418/diff/2/

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

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 

Re: Review Request 66411: Made the test CSI plugin compatible to CSI v0.2.

[Chun-Hung Hsiao]

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

(Updated April 11, 2018, 2:36 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Changes
---

Fixed a bug that made `DeleteVolume` nonidempotent.


Bugs: MESOS-8717
https://issues.apache.org/jira/browse/MESOS-8717


Repository: mesos


Description
---

This patch contains necessary changes for the test CSI plugin to support
CSI v0.2. The `STAGE_UNSTAGE_VOLUME` node service capability is not
implemented in this patch yet.


Diffs (updated)
-

  src/examples/test_csi_plugin.cpp aed22623b6dadb791eeaac2ecde4c31236a5fc19 


Diff: https://reviews.apache.org/r/66411/diff/3/

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


Testing
---

sudo make check


Thanks,

Chun-Hung Hsiao

Re: Review Request 66410: Adapted storage local resource provider to use CSI v0.2.

[Chun-Hung Hsiao]

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

(Updated April 11, 2018, 2:34 a.m.)


Review request for mesos, Benjamin Bannier and Jie Yu.


Bugs: MESOS-8717
https://issues.apache.org/jira/browse/MESOS-8717


Repository: mesos


Description
---

This patch contains necessary changes for the storage local resource
provider to use CSI v0.2. Support for the `STAGE_UNSTAGE_VOLUME` CSI
node service capability is not implemented in this patch yet.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
a07620d1c4bf618f669575b3e183bf598da905a6 


Diff: https://reviews.apache.org/r/66410/diff/3/

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


Testing
---

This patch cannot be compiled standalone. Tests done later in the chain.


Thanks,

Chun-Hung Hsiao

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 63367: Added overloads for strings::startsWith and strings::endsWith().

[Benjamin Mahler]

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




3rdparty/stout/include/stout/strings.hpp
Lines 390-392 (patched)


This is definitely simple but is this how C++20 std libraries implement 
this?

It seems non-optimal to always have to scan the entire prefix array for the 
null character? E.g. startsWith("foo", "bar ... <1MB>"). Ideally this could 
trip to false after checking the first character only?


- Benjamin Mahler


On April 3, 2018, 4:27 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63367/
> ---
> 
> (Updated April 3, 2018, 4:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This saves an unnecessary memory allocation when
> testing if a string starts or ends with a string literal,
> which accounts for almost all usages of these functions
> in Mesos and in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/strings.hpp 
> 067a7923c02342bccd9be1136a981fd6b0e0e9b4 
>   3rdparty/stout/tests/strings_tests.cpp 
> 395540aad88c76a66a43a54edfe9ca1a2d46d3b4 
> 
> 
> Diff: https://reviews.apache.org/r/63367/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 63367: Added overloads for strings::startsWith and strings::endsWith().

[Benjamin Mahler]

> On Jan. 24, 2018, 5:02 p.m., Michael Park wrote:
> > I would prefer a more general, systematic approach to this situation.
> > I really don't want to be deal with adding `string` and `const char []`
> > overloads everywhere. Ideally, we'd have a `string_view` class that we
> > could use instead of `string` here.
> > 
> > Now, as far as performance goes, there are a couple of things to note.
> >   (1) It seems like the claim is that it speeds up the tests? by how much?
> >   (2) The claim that we avoid an unnecessary allocation isn't actually
> >   true considering that the strings (at least in our tests) probably
> >   fit within the short-string optimization.
> >   (3) Is this change by itself a big win somewhere? or will it require
> >   a systematic change where `string` can be a `string_view`?
> >   If the former, I'd be fine with optimizing these functions, but
> >   if it's the former, I'd say we should invest in a `string_view` class.
> > 
> > I quite like performance, but some more evidence would be helpful here.
> > 
> > Now, under the assumption that the performance argument to be justified,
> > we can simply make the existing function a template and generalize the 
> > logic:
> > 
> > ```cpp
> > template 
> > bool startsWith(const string& s, const Stringish& prefix) {
> >   return s.size() >= distance(begin(prefix), end(prefix)) &&
> >  equal(begin(prefix), end(prefix), begin(s));
> > }
> > ```
> 
> Benno Evers wrote:
> I don't think the template version will work, unless there's an overload 
> of `std::end` for `const char*`? In general, the standard library also 
> includes overloads for `const char*` for most functions taking string 
> arguments (including the proposed `std::string_view::starts_with()`) so I 
> doubt there's some magic bullet to avoid that.
> 
> If we plan to embrace `string_view` in the future, we can still upgrade 
> the signature from `const char*` to `string_view`  once we made the switch to 
> C++17; nothing in this review makes it harder to introduce that. (unless you 
> want to avoid ABI breakage, but this would also prevent the template solution 
> above)
> 
> For the performance, I actually didn't do any measurements, but it seems 
> reasonable to assume that doing something will take more time than doing 
> nothing. Even if the string will be SSO-able on many platforms (not on all 
> though, we still build for e.g. Ubuntu 14.04 where the C++11 ABI isn't used 
> by default), it still has to be constructed first, which isn't free: 
> https://godbolt.org/g/SUJ9Tn
> 
> When testing this function in isolation, I can see about 30% speedup. In 
> the context of mesos, I don't expect to see a huge impact, but even a free 
> 0.1% would be nice to have, in particular in a function that was subject to 
> performance concerns in the past. (MESOS-5715)
> 
> Finally, having written all of this, my main motivation for this patch 
> was actually purely aesthetic: Paying a non-zero overhead to use an 
> abstraction just doesn't feel right in C++, no matter how small the practical 
> implications. ;)

Since C++20 will include starts_with and ends_with, it seems ok to just mimic 
the C++20 signatures now and put a TODO to remove these when we migrate to 
C++20.


- Benjamin


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


On April 3, 2018, 4:27 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63367/
> ---
> 
> (Updated April 3, 2018, 4:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This saves an unnecessary memory allocation when
> testing if a string starts or ends with a string literal,
> which accounts for almost all usages of these functions
> in Mesos and in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/strings.hpp 
> 067a7923c02342bccd9be1136a981fd6b0e0e9b4 
>   3rdparty/stout/tests/strings_tests.cpp 
> 395540aad88c76a66a43a54edfe9ca1a2d46d3b4 
> 
> 
> Diff: https://reviews.apache.org/r/63367/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Benno Evers
> 
>

Re: Review Request 65334: Added quota limit to the master API protos.

[Benjamin Mahler]

> On March 18, 2018, 8:27 p.m., Meng Zhu wrote:
> > include/mesos/master/master.proto
> > Lines 206 (patched)
> > 
> >
> > Why we are making it a repeated field? What is the usage case?

Much like `UpdateWeights`, the use case is to atomically update quota for 
multiple roles. Otherwise, we expose a races and the user has to work around 
them.

For example, I want to transfer 10cpus of "a"'s quota to "b". With this API I 
can directly update both in the same call, and the update will occur in an 
all-or-nothing manner. Without this, I should probably increase "b"'s quota 
first to avoid someone else taking the excess quickly after I reduce "a". This 
puts the cluster potentially in a temporary overcommit, and if the master fails 
or if my tooling fail, I got stuck halfway and need to figure out how to undo 
or recover from the failure and proceed.


- Benjamin


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


On March 10, 2018, 1:46 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65334/
> ---
> 
> (Updated March 10, 2018, 1:46 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Meng Zhu.
> 
> 
> Bugs: MESOS-8487
> https://issues.apache.org/jira/browse/MESOS-8487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This introduces a `limit` in `QuotaInfo` and `QuotaRequest`.
> 
> A new `UPDATE_QUOTA` master call is added as a replacement of
> `SET_QUOTA` and `REMOVE_QUOTA`, which provides the new semantics
> of unset limit meaning "no limit" and unset guarantee meaning
> "no guarantee".
> 
> The new APIs are marked as experimental for now.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 6622e1b82861f380346f505eca326f7174dd8bd6 
>   include/mesos/quota/quota.proto f2ed6f555e1e7b0290fefa3e301c9bdbf550bd11 
>   include/mesos/v1/master/master.proto 
> 6034bd5af8ad764f625f9fe80be08b48707bbadb 
>   include/mesos/v1/quota/quota.proto 5dd772fee46f740b062827ed7c9704c26187cb12 
>   src/master/http.cpp cf03d8bf7df583c849a3b2a326d3772820181b0f 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
> 
> 
> Diff: https://reviews.apache.org/r/65334/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66468: Added tests for operation status reconciliation.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66458, 66459, 66460, 66461, 66489, 66462, 66463, 66464, 
66465, 66466, 66467, 66468]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 10, 2018, 9:56 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66468/
> ---
> 
> (Updated April 10, 2018, 9:56 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for operation status reconciliation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
>   src/tests/operation_reconciliation_tests.cpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 2872f1aec1a7b94fc302a533f5ae9e1be9658087 
> 
> 
> Diff: https://reviews.apache.org/r/66468/diff/4/
> 
> 
> Testing
> ---
> 
> The new tests passed 1000 iterations on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 65334: Added quota limit to the master API protos.

[Benjamin Mahler]

> On March 18, 2018, 6:27 p.m., Meng Zhu wrote:
> > include/mesos/v1/quota/quota.proto
> > Lines 46-48 (original), 67-69 (patched)
> > 
> >
> > If I understand it correctly,
> > 
> > for guarantee check, we check the set guarantee < (parent's guarantee - 
> > sum of its siblings' guarantee). In the case of top-level role, its 
> > parent's guarantee is the total cluster size.
> > 
> > for limit check, we check the set limit < parent's limit. In the case 
> > of top-level role, its parent's limit is the total cluster size.
> > 
> > The "overcommiting the cluster" description here is rather vague and as 
> > a reader, I probably won't get the subtle difference between limit check 
> > and guarantee check.
> > 
> > How about being more explicit and provide the exact formula here like 
> > metioned above?

We do not yet have the parent/child case, this patch continues to assume flat 
roles for quota.


> On March 18, 2018, 6:27 p.m., Meng Zhu wrote:
> > include/mesos/v1/quota/quota.proto
> > Lines 71-73 (original), 92-94 (patched)
> > 
> >
> > "exceed the total cluster size" is not accurate, see comments above.

ditto here


- Benjamin


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


On March 10, 2018, 1:46 a.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65334/
> ---
> 
> (Updated March 10, 2018, 1:46 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Michael Park, and Meng Zhu.
> 
> 
> Bugs: MESOS-8487
> https://issues.apache.org/jira/browse/MESOS-8487
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This introduces a `limit` in `QuotaInfo` and `QuotaRequest`.
> 
> A new `UPDATE_QUOTA` master call is added as a replacement of
> `SET_QUOTA` and `REMOVE_QUOTA`, which provides the new semantics
> of unset limit meaning "no limit" and unset guarantee meaning
> "no guarantee".
> 
> The new APIs are marked as experimental for now.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 6622e1b82861f380346f505eca326f7174dd8bd6 
>   include/mesos/quota/quota.proto f2ed6f555e1e7b0290fefa3e301c9bdbf550bd11 
>   include/mesos/v1/master/master.proto 
> 6034bd5af8ad764f625f9fe80be08b48707bbadb 
>   include/mesos/v1/quota/quota.proto 5dd772fee46f740b062827ed7c9704c26187cb12 
>   src/master/http.cpp cf03d8bf7df583c849a3b2a326d3772820181b0f 
>   src/master/validation.cpp 9254cd22e67d9aca53135701575fb0857ff6f7d6 
> 
> 
> Diff: https://reviews.apache.org/r/65334/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 66541: Added default executor test for agent recovery without metadata.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66541 was successfully built and tested.

Reviews applied: `['66538', '66539', '66540', '66541']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66541

- Mesos Reviewbot Windows


On April 10, 2018, 11:41 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66541/
> ---
> 
> (Updated April 10, 2018, 11:41 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8416
> https://issues.apache.org/jira/browse/MESOS-8416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added default executor test for agent recovery without metadata.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> 293dd20d882447401572835bd31e197faf76861b 
> 
> 
> Diff: https://reviews.apache.org/r/66541/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> Verified that the test crashes without the fix in r/66539.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Benjamin Mahler]

> On April 10, 2018, 10 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1607-1610 (patched)
> > 
> >
> > I assume this continues to work as the value scalar goes negative? We 
> > should probably pull this up into the header and unit test it?
> 
> Meng Zhu wrote:
> By "continues to work", I guess you expect the function to ignore 
> negative shrink? Then no, current `Resources::shrink()` will return a 
> `Resource` object with negative values with the same meta-data (if 
> shrinkable).
> 
> 
> https://github.com/apache/mesos/blob/88f5629e510d71a32bd7e0ff7ee09e150f944e72/src/v1/resources.cpp#L1296-L1315
> 
> I can fix that in a separate patch (if negative return directly).
> 
> Though I think in most use cases, the scalar values will most likely come 
> from another resource(s) object, I do not think a negative resource object 
> can be created easily (unless intensionally).

Oh I see now that the loop won't produce negative scalars, nevermind :)


- Benjamin


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


On April 10, 2018, 10:54 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated April 10, 2018, 10:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 32e88952101d8dbbae9728478b1f5663bf46c3bb 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66539: Fixed the agent recovery crash if metadata is missing.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 10, 2018, 11:41 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66539/
> ---
> 
> (Updated April 10, 2018, 11:41 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.
> 
> 
> Bugs: MESOS-8416
> https://issues.apache.org/jira/browse/MESOS-8416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the case that is missed when handling orphan containers
> cleanup. When the agent metadata does not exist but the container
> pid is chechpointed under the container runtime dir, then the
> container should be regarded as orphan and should be cleaned up.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 7473a871e626833733f39375b778aff70529dc63 
> 
> 
> Diff: https://reviews.apache.org/r/66539/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 66540: Added unit test for recovering nested container without slave state.

[Gilbert Song]

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

Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.


Bugs: MESOS-8416
https://issues.apache.org/jira/browse/MESOS-8416


Repository: mesos


Description
---

Added unit test for recovering nested container without slave state.


Diffs
-

  src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
796bc401d4f922e7a6bd9e5391003cddd4331c95 


Diff: https://reviews.apache.org/r/66540/diff/1/


Testing
---

make check

Verified that the test crashes without the fix in r/66539.


Thanks,

Gilbert Song

Review Request 66538: Added unit test slave recovery for default executor tests.

[Gilbert Song]

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

Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.


Bugs: MESOS-8772
https://issues.apache.org/jira/browse/MESOS-8772


Repository: mesos


Description
---

Added unit test slave recovery for default executor tests.


Diffs
-

  src/tests/default_executor_tests.cpp 293dd20d882447401572835bd31e197faf76861b 


Diff: https://reviews.apache.org/r/66538/diff/1/


Testing
---

make check


Thanks,

Gilbert Song

Review Request 66541: Added default executor test for agent recovery without metadata.

[Gilbert Song]

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

Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.


Bugs: MESOS-8416
https://issues.apache.org/jira/browse/MESOS-8416


Repository: mesos


Description
---

Added default executor test for agent recovery without metadata.


Diffs
-

  src/tests/default_executor_tests.cpp 293dd20d882447401572835bd31e197faf76861b 


Diff: https://reviews.apache.org/r/66541/diff/1/


Testing
---

make check

Verified that the test crashes without the fix in r/66539.


Thanks,

Gilbert Song

Review Request 66539: Fixed the agent recovery crash if metadata is missing.

[Gilbert Song]

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

Review request for mesos, Jie Yu, Kevin Klues, Qian Zhang, and Vinod Kone.


Bugs: MESOS-8416
https://issues.apache.org/jira/browse/MESOS-8416


Repository: mesos


Description
---

This is the case that is missed when handling orphan containers
cleanup. When the agent metadata does not exist but the container
pid is chechpointed under the container runtime dir, then the
container should be regarded as orphan and should be cleaned up.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
7473a871e626833733f39375b778aff70529dc63 


Diff: https://reviews.apache.org/r/66539/diff/1/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 66468: Added tests for operation status reconciliation.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66468 was successfully built and tested.

Reviews applied: `['66458', '66459', '66460', '66461', '66489', '66462', 
'66463', '66464', '66465', '66466', '66467', '66468']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66468

- Mesos Reviewbot Windows


On April 10, 2018, 9:56 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66468/
> ---
> 
> (Updated April 10, 2018, 9:56 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for operation status reconciliation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
>   src/tests/operation_reconciliation_tests.cpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 2872f1aec1a7b94fc302a533f5ae9e1be9658087 
> 
> 
> Diff: https://reviews.apache.org/r/66468/diff/4/
> 
> 
> Testing
> ---
> 
> The new tests passed 1000 iterations on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Meng Zhu]

> On April 10, 2018, 3 p.m., Benjamin Mahler wrote:
> > Looks good! Just thinking we probably should pull the shrink function up 
> > and unit test it? Feel free to do that in a second patch.

Got it. Will put it in the resource headers in a followup patch.


> On April 10, 2018, 3 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1600 (patched)
> > 
> >
> > To enable move performance improvements later, we can do:
> > 
> > ```
> > result += std::move(resource);
> > ```

Done.


> On April 10, 2018, 3 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1607-1610 (patched)
> > 
> >
> > I assume this continues to work as the value scalar goes negative? We 
> > should probably pull this up into the header and unit test it?

By "continues to work", I guess you expect the function to ignore negative 
shrink? Then no, current `Resources::shrink()` will return a `Resource` object 
with negative values with the same meta-data (if shrinkable).

https://github.com/apache/mesos/blob/88f5629e510d71a32bd7e0ff7ee09e150f944e72/src/v1/resources.cpp#L1296-L1315

I can fix that in a separate patch (if negative return directly).

Though I think in most use cases, the scalar values will most likely come from 
another resource(s) object, I do not think a negative resource object can be 
created easily (unless intensionally).


- Meng


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


On April 10, 2018, 3:54 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated April 10, 2018, 3:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 32e88952101d8dbbae9728478b1f5663bf46c3bb 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Meng Zhu]

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

(Updated April 10, 2018, 3:54 p.m.)


Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael Park, 
and Till Toenshoff.


Repository: mesos


Description
---

Introduced a `shrinkResources()` lambda in allocator
so that the same resource chopping logic can be re-used
in the future, in particular, when introducing the quota
limit.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
32e88952101d8dbbae9728478b1f5663bf46c3bb 


Diff: https://reviews.apache.org/r/66044/diff/3/

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


Testing
---

make check


Thanks,

Meng Zhu

Re: Review Request 66044: Refactored resources chopping logic in allocator.

[Benjamin Mahler]

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


Fix it, then Ship it!




Looks good! Just thinking we probably should pull the shrink function up and 
unit test it? Feel free to do that in a second patch.


src/master/allocator/mesos/hierarchical.cpp
Lines 1600 (patched)


To enable move performance improvements later, we can do:

```
result += std::move(resource);
```



src/master/allocator/mesos/hierarchical.cpp
Lines 1607-1610 (patched)


I assume this continues to work as the value scalar goes negative? We 
should probably pull this up into the header and unit test it?



src/master/allocator/mesos/hierarchical.cpp
Lines 1608-1609 (patched)


Ditto here for moves:

```
targetScalarQuantites[resource.name()] -= limitScalar.get();
result += std::move(resource);
```



src/master/allocator/mesos/hierarchical.cpp
Line 1868 (original), 1883 (patched)


s/(/ (/



src/master/allocator/mesos/hierarchical.cpp
Line 1870 (original), 1885 (patched)


Can you use CHECK_NOTNONE here instead of .get()?



src/master/allocator/mesos/hierarchical.cpp
Lines 1875-1876 (original), 1888-1889 (patched)


I think it's clear from the code without this comment, the comment actually 
threw me off because I was trying to figure out what "this time" was referring 
to (e.g. stage 1?).



src/master/allocator/mesos/hierarchical.cpp
Lines 1927 (patched)


Ditto here s/(/ (/



src/master/allocator/mesos/hierarchical.cpp
Lines 1929 (patched)


Ditto here CHECK_NOTNONE


- Benjamin Mahler


On March 19, 2018, 11:48 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66044/
> ---
> 
> (Updated March 19, 2018, 11:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kapil Arya, Joseph Wu, Michael 
> Park, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a `shrinkResources()` lambda in allocator
> so that the same resource chopping logic can be re-used
> in the future, in particular, when introducing the quota
> limit.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 32e88952101d8dbbae9728478b1f5663bf46c3bb 
> 
> 
> Diff: https://reviews.apache.org/r/66044/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 66468: Added tests for operation status reconciliation.

[Gaston Kleiman]

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

(Updated April 10, 2018, 2:56 p.m.)


Review request for mesos and Greg Mann.


Changes
---

Disabled on Windows a test that requires the replicated log, which isn't 
supported on Windwos.


Repository: mesos


Description
---

Added tests for operation status reconciliation.


Diffs (updated)
-

  src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
  src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
  src/tests/operation_reconciliation_tests.cpp PRE-CREATION 
  src/tests/storage_local_resource_provider_tests.cpp 
2872f1aec1a7b94fc302a533f5ae9e1be9658087 


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

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


Testing
---

The new tests passed 1000 iterations on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 66465: Updated `using` statements in `tests/mesos.hpp`.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 5, 2018, 12:48 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66465/
> ---
> 
> (Updated April 5, 2018, 12:48 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `using` statements in `tests/mesos.hpp`.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 46c271b5c5bedbdabd58b3cdbb82216d55c846bd 
> 
> 
> Diff: https://reviews.apache.org/r/66465/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> The tests added by https://reviews.apache.org/r/66468/ use these statements.
> 
> 
> 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 66467: Added a test helper for creating `RECONCILE_OPERATIONS` v1 calls.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 5, 2018, 12:50 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66467/
> ---
> 
> (Updated April 5, 2018, 12:50 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test helper for creating `RECONCILE_OPERATIONS` v1 calls.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 46c271b5c5bedbdabd58b3cdbb82216d55c846bd 
> 
> 
> Diff: https://reviews.apache.org/r/66467/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> The tests added by https://reviews.apache.org/r/66468/ use this helper.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66466: Updated `RESERVE()` helper to allow specifying an operation ID.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 5, 2018, 12:49 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66466/
> ---
> 
> (Updated April 5, 2018, 12:49 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `RESERVE()` helper to allow specifying an operation ID.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 46c271b5c5bedbdabd58b3cdbb82216d55c846bd 
> 
> 
> Diff: https://reviews.apache.org/r/66466/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> The tests added by https://reviews.apache.org/r/66468/ use this helper with 
> an `OperationID`.
> 
> 
> 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 66468: Added tests for operation status reconciliation.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66458, 66459, 66460, 66461, 66489, 66462, 66463, 66464, 
66465, 66466, 66467, 66468]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 10, 2018, 11:45 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66468/
> ---
> 
> (Updated April 10, 2018, 11:45 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for operation status reconciliation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
>   src/tests/operation_reconciliation_tests.cpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 2872f1aec1a7b94fc302a533f5ae9e1be9658087 
> 
> 
> Diff: https://reviews.apache.org/r/66468/diff/3/
> 
> 
> Testing
> ---
> 
> The new tests passed 1000 iterations 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 66532: Added test for authorization actions for `UPDATE_VOLUME`.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66218', '66049', '66050', '66219', '66220', '66052', 
'66051', '66227', '66531', '66532']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66532

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66532/logs/mesos-tests-stdout.log):

```
[--] 9 tests from Endpoint/SlaveEndpointTest (1075 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (35 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (39 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (76 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (781 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (808 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (838 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (867 ms total)

[--] Global test environment tear-down
[==] 957 tests from 94 test cases ran. (467339 ms total)
[  PASSED  ] 955 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] 
ContentType/MasterAPITest.CreateGrowShrinkDestroyPersistentVolume/0, where 
GetParam() = application/x-protobuf
[  FAILED  ] 
ContentType/MasterAPITest.CreateGrowShrinkDestroyPersistentVolume/1, where 
GetParam() = application/json

 2 FAILED TESTS
  YOU HAVE 214 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66532/logs/mesos-tests-stderr.log):

```
I0410 20:39:35.408179 22096 executor.cpp:177] Received SUBSCRIBED event
I0410 20:39:35.413197 22096 executor.cpp:181] Subscribed executor on 
winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0410 20:39:35.413197 22096 executor.cpp:177] Received LAUNCH event
I0410 20:39:35.418190 22096 executor.cpp:649] Starting task 
ce703cd1-acd0-4725-9dd6-e47ab31e4432
I0410 20:39:35.509244 22096 executor.cpp:484] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0410 20:39:35.540205 22096 executor.cpp:662] Forked command at 21472
I0410 20:39:35.573182 21000 exec.cpp:445] Executor asked to shutdown
I0410 20:39:35.573182 21652 executor.cpp:177] Received SHUTDOWN event
I0410 20:39:35.573182 21652 executor.cpp:759] Shutting down
I0410 20:39:35.573182 21652 executor.cpp:869] Sending SIGTERM to process tree 
at pid .569183  6056 master.cpp:3249] Deactivating framework 
c6ab24ad-96d4-4f3c-b927-4659302996a0- (default) at 
scheduler-2540719b-1b4b-470f-8809-e33a0b4a52bb@10.3.1.8:60882
I0410 20:39:35.571203 22988 hierarchical.cpp:405] Deactivated framework 
c6ab24ad-96d4-4f3c-b927-4659302996a0-
I0410 20:39:35.571203 21372 slave.cpp:3973] Shutting down framework 
c6ab24ad-96d4-4f3c-b927-4659302996a0-
I0410 20:39:35.571203  6056 master.cpp:10646] Updating the state of task 
ce703cd1-acd0-4725-9dd6-e47ab31e4432 of framework 
c6ab24ad-96d4-4f3c-b927-4659302996a0- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0410 20:39:35.571203 21372 slave.cpp:6682] Shutting down executor 
'ce703cd1-acd0-4725-9dd6-e47ab31e4432' of framework 
c6ab24ad-96d4-4f3c-b927-4659302996a0- at executor(1)@10.3.1.8:60903
I0410 20:39:35.572187 21372 slave.cpp:923] Agent terminating
W0410 20:39:35.572187 21372 slave.cpp:3969] Ignoring shutdown framework 
c6ab24ad-96d4-4f3c-b927-4659302996a0- because it is terminating
I0410 20:39:35.575168  6056 master.cpp:10745] Removing task 
ce703cd1-acd0-4725-9dd6-e47ab31e4432 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework c6ab24ad-96d4-4f3c-b927-4659302996a0- on 
agent c6ab24ad-96d4-4f3c-b927-4659302996a0-S0 at slave(425)@10.3.1.8:60882 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0410 20:39:35.575168 21372 containerizer.cpp:2338] Destroying container 
936fac82-7c94-42ed-beb9-144facfa1da0 in RUNNING state
I0410 20:39:35.576180 21372 containerizer.cpp:2952] Transitioning the state of 
container 936fac82-7c94-42ed-beb9-144facfa1da0 from RUNNING to DESTROYING
I0410 20:39:35.577185 21372 launcher.cpp:156] Asked to 

Re: Review Request 66468: Added tests for operation status reconciliation.

[Mesos Reviewbot Windows]

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



FAIL: Some of the unit tests failed. Please check the relevant logs.

Reviews applied: `['66458', '66459', '66460', '66461', '66489', '66462', 
'66463', '66464', '66465', '66466', '66467', '66468']`

Failed command: `Start-MesosCITesting`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66468

Relevant logs:

- 
[mesos-tests-stdout.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66468/logs/mesos-tests-stdout.log):

```
[--] 9 tests from Endpoint/SlaveEndpointTest (1104 ms total)

[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/0 (32 
ms)
[ RUN  ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1
[   OK ] ContainerizerType/DefaultContainerDNSFlagTest.ValidateFlag/1 (36 
ms)
[--] 2 tests from ContainerizerType/DefaultContainerDNSFlagTest (71 ms 
total)

[--] 1 test from IsolationFlag/CpuIsolatorTest
[ RUN  ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0
[   OK ] IsolationFlag/CpuIsolatorTest.ROOT_UserCpuUsage/0 (796 ms)
[--] 1 test from IsolationFlag/CpuIsolatorTest (824 ms total)

[--] 1 test from IsolationFlag/MemoryIsolatorTest
[ RUN  ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0
[   OK ] IsolationFlag/MemoryIsolatorTest.ROOT_MemUsage/0 (813 ms)
[--] 1 test from IsolationFlag/MemoryIsolatorTest (839 ms total)

[--] Global test environment tear-down
[==] 968 tests from 95 test cases ran. (439901 ms total)
[  PASSED  ] 966 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] 
ContentType/OperationReconciliationTest.UnknownOperationRecoveredAgent/0, where 
GetParam() = application/x-protobuf
[  FAILED  ] 
ContentType/OperationReconciliationTest.UnknownOperationRecoveredAgent/1, where 
GetParam() = application/json

 2 FAILED TESTS
  YOU HAVE 214 DISABLED TESTS

```

- 
[mesos-tests-stderr.log](http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66468/logs/mesos-tests-stderr.log):

```
II0410 19:45:45.065649 22136 exec.cpp:162] Version: 1.6.0
I0410 19:45:45.093673 23124 exec.cpp:236] Executor registered on agent 
e2a54159-1b89-4598-80c7-6aad0a63e716-S0
I0410 19:45:45.097678 21904 executor.cpp:177] Received SUBSCRIBED event
I0410 19:45:45.102671 21904 executor.cpp:181] Subscribed executor on 
winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net
I0410 19:45:45.102671 21904 executor.cpp:177] Received LAUNCH event
I0410 19:45:45.107671 21904 executor.cpp:649] Starting task 
f428fa3b-5afa-4aa6-8d12-f35afe8f3578
I0410 19:45:45.197675 21904 executor.cpp:484] Running 
'D:\DCOS\mesos\src\mesos-containerizer.exe launch '
I0410 19:45:45.230665 21904 executor.cpp:662] Forked command at 18632
I0410 19:45:45.266690 10936 exec.cpp:445] Executor asked to shutdown
I0410 19:45:45.267647 22416 executor.cpp:177] Received SHUTDOWN event
I0410 19:45:45.267647 22416 executor.cpp:759] Shutting down
I0410 19:45:45.267647 22416 executor.cpp:869] Sending SIGTERM to process tree 
at pid 0410 19:45:45.263679 19476 master.cpp:10567] Updating the state of task 
f428fa3b-5afa-4aa6-8d12-f35afe8f3578 of framework 
e2a54159-1b89-4598-80c7-6aad0a63e716- (latest state: TASK_KILLED, status 
update state: TASK_KILLED)
I0410 19:45:45.263679 22892 slave.cpp:3973] Shutting down framework 
e2a54159-1b89-4598-80c7-6aad0a63e716-
I0410 19:45:45.264657 22892 slave.cpp:6670] Shutting down executor 
'f428fa3b-5afa-4aa6-8d12-f35afe8f3578' of framework 
e2a54159-1b89-4598-80c7-6aad0a63e716- at executor(1)@10.3.1.8:58481
I0410 19:45:45.266690 11396 slave.cpp:923] Agent terminating
W0410 19:45:45.266690 11396 slave.cpp:3969] Ignoring shutdown framework 
e2a54159-1b89-4598-80c7-6aad0a63e716- because it is terminating
I0410 19:45:45.268725 19476 master.cpp:10666] Removing task 
f428fa3b-5afa-4aa6-8d12-f35afe8f3578 with resources cpus(allocated: *):4; 
mem(allocated: *):2048; disk(allocated: *):1024; ports(allocated: 
*):[31000-32000] of framework e2a54159-1b89-4598-80c7-6aad0a63e716- on 
agent e2a54159-1b89-4598-80c7-6aad0a63e716-S0 at slave(433)@10.3.1.8:58460 
(winbldsrv-01.zq4gs31qjdiunm1ryi1452nvnh.dx.internal.cloudapp.net)
I0410 19:45:45.270648 14944 containerizer.cpp:2338] Destroying container 
ccb6f3b3-c36b-4aa1-b463-d1d15b3ad8c1 in RUNNING state
I0410 19:45:45.270648 14944 containerizer.cpp:2952] Transitioning the state of 
container ccb6f3b3-c36b-4aa1-b463-d1d15b3ad8c1 from RUNNING to DESTROYING
I0410 19:45:45.271651 14944 launcher.cpp:156] Asked to destroy container 
ccb6f3b3-c36b-4aa1-b463-d1d15b3ad8c1
I0410 19:45:45.271651 19476 

Review Request 66532: Added test for authorization actions for `UPDATE_VOLUME`.

[Zhitao Li]

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

Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

Added test for authorization actions for `UPDATE_VOLUME`.


Diffs
-

  src/tests/authorization_tests.cpp a76ad18a54ec232e6d6c92ec6a45b445a83f174c 


Diff: https://reviews.apache.org/r/66532/diff/1/


Testing
---


Thanks,

Zhitao Li

Review Request 66531: Added new authorization for `UpdateVolume`.

[Zhitao Li]

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

Review request for mesos, Chun-Hung Hsiao and Greg Mann.


Bugs: MESOS-8748
https://issues.apache.org/jira/browse/MESOS-8748


Repository: mesos


Description
---

The new authorization action is modelled after `CreateVolume`, and will
be shared by both `GROW_VOLUME` and `SHRINK_VOLUME` operations and
corresponding operator APIs.


Diffs
-

  include/mesos/authorizer/acls.proto 8ef33210644e7d2924b402a3158b1b38c1fdb464 
  include/mesos/authorizer/authorizer.proto 
1508c01130013d74ed2b2574a2428f38e3d2c064 
  src/authorizer/local/authorizer.cpp c098ba9ded1b29a2e079cf09ab80b61f6fa4af05 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 


Diff: https://reviews.apache.org/r/66531/diff/1/


Testing
---

Manually created ACL and verified that untrusted principals will not allow to 
grow/shrink volumes.
Also created unit test in next patch.


Thanks,

Zhitao Li

Re: Review Request 66437: Windows: Removed `FD_CRT` from `WindowsFD` abstraction.

[Andrew Schwartzmeyer]

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

(Updated April 10, 2018, 12:04 p.m.)


Review request for mesos, Akash Gupta, Eric Mumau, John Kordich, Joseph Wu, and 
Michael Park.


Changes
---

Updated file system tests.


Bugs: MESOS-8675 and MESOS-8683
https://issues.apache.org/jira/browse/MESOS-8675
https://issues.apache.org/jira/browse/MESOS-8683


Repository: mesos


Description (updated)
---

After all the CRT APIs were replaced with Windows APIs, we no longer
needed to support the semantics of an `int` file descriptor in
general (in the sense of opening a CRT handle that's associated with
the actual kernel object for the given `HANDLE`). There are specific
use cases (usually third-party code) which still require a CRT
int-like file descriptor, which the `crt()` function explicitly
allocates (this allocation used to be done in the constructor).

Thus the entire `FD_CRT` type was removed from the `WindowsFD`
abstraction. It still acts like an `int` in the sense that it can be
constructed from one and compared to one. However, construction via
`int` only supports the standard file descriptors 0, 1, and 2 for
`stdin`, `stdout`, and `stderr`. Any other construction creates an
`int_fd` which holds an `INVALID_HANDLE_VALUE`. When being compared to
an `int`, the abstraction simply returns -1 if it is invalid (based on
the result of the `is_valid()` method) or 0 if it is valid. This is to
support the semantics of checking validity by something like
`if (fd < 0)` or `if (fd == -1)`.

With the deletion of the `FD_CRT` type from `WindowsFD`, all the Stout
APIs that switched on the type were simplified, with the last of the
CRT code deleted.

Thanks to the introduction of the private `int get_valid()` function,
and the removal of the `FD_CRT` type, the comparison operators became
much simpler.

Several unit tests in the `FsTest` suite became cross-platform, with
the `Close` test being simplified to test against an `int_fd`.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/windows/close.hpp 
ff635e44235d63888a210cd68d49f6678a851e31 
  3rdparty/stout/include/stout/os/windows/dup.hpp 
265046cf7ffc14f7326711d295aa7dd4f0a8a1e3 
  3rdparty/stout/include/stout/os/windows/fcntl.hpp 
bf8c38acad60f9b0eb752053dcd53a9fda7b8bfa 
  3rdparty/stout/include/stout/os/windows/fd.hpp 
d7f8cdf1ad877eb55589bf5a9e75d295f91990a7 
  3rdparty/stout/include/stout/os/windows/read.hpp 
8047ad590fcc46d3ec46b551472d8c518ae49cc1 
  3rdparty/stout/include/stout/os/windows/write.hpp 
71006489918d9495d37d2fdfdca08b40b419481a 
  3rdparty/stout/tests/os/filesystem_tests.cpp 
c190baa2230298e428d4034b90dccffb59b4e710 


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

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


Testing
---


Thanks,

Andrew Schwartzmeyer

Re: Review Request 66462: Added new operation states to be used for status reconciliation.

[Greg Mann]

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


Fix it, then Ship it!





include/mesos/mesos.proto
Lines 2314 (patched)


s/being applied/pending/



include/mesos/mesos.proto
Lines 2322 (patched)


s/when/if/


- Greg Mann


On April 4, 2018, 11:58 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66462/
> ---
> 
> (Updated April 4, 2018, 11:58 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added new operation states to be used for status reconciliation.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/common/protobuf_utils.cpp 141a444534b776a2c90e2a0daf9727cd21e39080 
>   src/master/master.cpp f7da675e8fe96159e5335c9e83b65b67ed90eda8 
>   src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
> 
> 
> Diff: https://reviews.apache.org/r/66462/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66468: Added tests for operation status reconciliation.

[Gaston Kleiman]

> On April 9, 2018, 5:08 p.m., Greg Mann wrote:
> > src/tests/operation_reconciliation_tests.cpp
> > Lines 364-365 (patched)
> > 
> >
> > Is there a reason not to do this with a `for` loop?
> 
> Gaston Kleiman wrote:
> I copied & pasted this from 
> https://github.com/apache/mesos/blob/be47e96e727f07758ff9b8ba1c23bbec2a489cd6/src/tests/reconciliation_tests.cpp#L1221-L1230,
>  so consistency with that file would be the only reason =).
> 
> Doing this with a `for` loop calls `FUTURE_MESSAGE` and `Clock::advance` 
> one extra time, but that doesn't really hurt, so I converted the `while` loop 
> into a `for` loop. Let me know if you like it this way or if you'd prefer me 
> to revert this change.

I changed it back to a `while` loop as discussed in chat.


- Gaston


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


On April 10, 2018, 11:45 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66468/
> ---
> 
> (Updated April 10, 2018, 11:45 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for operation status reconciliation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
>   src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
>   src/tests/operation_reconciliation_tests.cpp PRE-CREATION 
>   src/tests/storage_local_resource_provider_tests.cpp 
> 2872f1aec1a7b94fc302a533f5ae9e1be9658087 
> 
> 
> Diff: https://reviews.apache.org/r/66468/diff/3/
> 
> 
> Testing
> ---
> 
> The new tests passed 1000 iterations on GNU/Linux.
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66468: Added tests for operation status reconciliation.

[Gaston Kleiman]

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

(Updated April 10, 2018, 11:45 a.m.)


Review request for mesos and Greg Mann.


Repository: mesos


Description
---

Added tests for operation status reconciliation.


Diffs (updated)
-

  src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
  src/tests/CMakeLists.txt ade5180f6e072112ad4836aa436e8545260d2ae6 
  src/tests/operation_reconciliation_tests.cpp PRE-CREATION 
  src/tests/storage_local_resource_provider_tests.cpp 
2872f1aec1a7b94fc302a533f5ae9e1be9658087 


Diff: https://reviews.apache.org/r/66468/diff/3/

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


Testing
---

The new tests passed 1000 iterations on GNU/Linux.


Thanks,

Gaston Kleiman

Re: Review Request 66489: Cleaned up internal evolve functions.

[Gaston Kleiman]

> On April 10, 2018, 11:39 a.m., Greg Mann wrote:
> > src/internal/evolve.hpp
> > Line 152 (original), 152-153 (patched)
> > 
> >
> > Can we get rid of the `mesos::` here as well?

Nope, if we get rid of `mesos::` here, it will complain about not finding 
`mesos::internal::master::Response`:

```
../../src/internal/evolve.hpp:152:40: error: ‘Event’ in namespace 
‘mesos::internal::master’ does not name a type
 v1::master::Event evolve(const master::Event& event);
^
../../src/internal/evolve.hpp:153:43: error: ‘Response’ in namespace 
‘mesos::internal::master’ does not name a type
 v1::master::Response evolve(const master::Response& response);
   ^~~~
```


- Gaston


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


On April 6, 2018, 5:16 p.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66489/
> ---
> 
> (Updated April 6, 2018, 5:16 p.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removes the `mesos::` prefix from declarations where
> possible, uses a consistent number of empty lines in the header,
> and reorders some declarations.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 
>   src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 
> 
> 
> Diff: https://reviews.apache.org/r/66489/diff/1/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66489: Cleaned up internal evolve functions.

[Greg Mann]

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


Fix it, then Ship it!





src/internal/evolve.hpp
Line 152 (original), 152-153 (patched)


Can we get rid of the `mesos::` here as well?


- Greg Mann


On April 7, 2018, 12:16 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66489/
> ---
> 
> (Updated April 7, 2018, 12:16 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removes the `mesos::` prefix from declarations where
> possible, uses a consistent number of empty lines in the header,
> and reorders some declarations.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 
>   src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 
> 
> 
> Diff: https://reviews.apache.org/r/66489/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]

> 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 66461: Added an evolve function for `v1::scheduler::Response`.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On April 7, 2018, 12:16 a.m., Gaston Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66461/
> ---
> 
> (Updated April 7, 2018, 12:16 a.m.)
> 
> 
> Review request for mesos and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added an evolve function for `v1::scheduler::Response`.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp e00ac7175438f758b10c0aa9485fc29ffa4efa29 
>   src/internal/evolve.cpp f598ea11b64bdbface5bf1f124a02ea876196b67 
> 
> 
> Diff: https://reviews.apache.org/r/66461/diff/2/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests` on GNU/Linux
> 
> 
> Thanks,
> 
> Gaston Kleiman
> 
>

Re: Review Request 66511: Updated tests due to change of containerizer's `destroy()` return type.

[Greg Mann]

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




src/tests/containerizer.cpp
Lines 534-535 (original), 535-537 (patched)


Indentation, here and elsewhere:
```
  Future (
  TestContainerizerProcess::*destroy)(const ContainerID&) =
::destroy;
```



src/tests/containerizer/composing_containerizer_tests.cpp
Line 118 (original), 119 (patched)


Is it not possible to simply use `None()` here and elsewhere?


- Greg Mann


On April 9, 2018, 3:45 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66511/
> ---
> 
> (Updated April 9, 2018, 3:45 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8710
> https://issues.apache.org/jira/browse/MESOS-8710
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As the return type of `destroy()` method has been changed, we need to
> update tests accordingly. This patch fixes all tests affected by the
> change.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 97dde00bbd32d763c961e6ad2b323fb0ca3d774f 
>   src/tests/containerizer.hpp d6a69d6327018065453f8a6cb275d921d10c9198 
>   src/tests/containerizer.cpp 13a290ff60e359ceced95b20cb9db8c25cd59dbc 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> 09f7ea42dd833c36aefdb073517d809431c4f328 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> c198ecbac50ef4eb7ea243bebc17b65db7ce5cb7 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 8fdaa7ffb11d3cc48e1a2f4eaa086adda2837106 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 4fce8af7c89fd3b9166e1784203f99853624faef 
>   src/tests/containerizer/mock_containerizer.hpp 
> 01c617a55899a7125ca1dc80091aef5603419ee8 
> 
> 
> Diff: https://reviews.apache.org/r/66511/diff/1/
> 
> 
> Testing
> ---
> 
> internal CI
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 66510: Unified return type of `wait` and `destroy` containerizer methods.

[Greg Mann]

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




src/slave/containerizer/composing.cpp
Line 376 (original), 378 (patched)


Is it not possible to use `None()` here instead of 
`Option::none()`? Here and elsewhere.



src/slave/containerizer/mesos/containerizer.cpp
Line 2335 (original), 2336 (patched)


Hmm I think it might be a bit more readable here and elsewhere to continue 
using a trivial lambda which returns a default-constructed 
`ContainerTermination`, WDYT?

```
.then([]() { return ContainerTermination(); });
```


- Greg Mann


On April 9, 2018, 3:44 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66510/
> ---
> 
> (Updated April 9, 2018, 3:44 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian 
> Zhang.
> 
> 
> Bugs: MESOS-8706
> https://issues.apache.org/jira/browse/MESOS-8706
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the return type of `destroy()` method for all
> containerizers. As a result, `destroy()` and `wait()` methods have
> got the same signature and return type. In fact, all these methods
> depend on the same container termination promise, so this unification
> makes both containerizer interface and internal logic more consistent.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> 1221d9b61b7cfbdbaa560dbafb0158225331ed6d 
>   src/slave/containerizer/composing.cpp 
> cd840a5409023d32701329bf37806f2e7ed6ffd2 
>   src/slave/containerizer/containerizer.hpp 
> 836283aa8aa827459558d567e090a230c32351ed 
>   src/slave/containerizer/docker.hpp f1b56c8edced17e9786c86adb9b92c829ed81635 
>   src/slave/containerizer/docker.cpp 31d64a7ea2a331a084c0f1707b82e938dfe6390f 
>   src/slave/containerizer/mesos/containerizer.hpp 
> cba4ed2e20622070b46071f35907d8b32c25b2fc 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 7473a871e626833733f39375b778aff70529dc63 
>   src/slave/http.cpp d500fde22d01d2c66104b72ff39d9de4e3a411cd 
> 
> 
> Diff: https://reviews.apache.org/r/66510/diff/1/
> 
> 
> Testing
> ---
> 
> Tests can't be compiled after this change.
> See the next patch in the chain.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 66527: Avoided copy in accessing resource provider subscription field.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66527 was successfully built and tested.

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66527

- Mesos Reviewbot Windows


On April 10, 2018, 5:56 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66527/
> ---
> 
> (Updated April 10, 2018, 5:56 a.m.)
> 
> 
> Review request for mesos and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided copy in accessing resource provider subscription field.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
> 
> 
> Diff: https://reviews.apache.org/r/66527/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66528: Added logging of failed resource provider registry updates.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66528 was successfully built and tested.

Reviews applied: `['66528']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66528

- Mesos Reviewbot Windows


On April 10, 2018, 8:05 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66528/
> ---
> 
> (Updated April 10, 2018, 8:05 a.m.)
> 
> 
> Review request for mesos and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added logging of failed resource provider registry updates.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.cpp 
> 9eb49f1327a0b598c5464a29a09ee286d7018f09 
> 
> 
> Diff: https://reviews.apache.org/r/66528/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66311: Implement recovery of resource provider manager.

[Benjamin Bannier]

> On April 10, 2018, 3:05 p.m., Jan Schlicht wrote:
> >

Thank you for your comments. I have some issues updating the patch in 
reviewboard at this time, will retry later. Please see my responses below.


> On April 10, 2018, 3:05 p.m., Jan Schlicht wrote:
> > src/resource_provider/manager.cpp
> > Lines 201 (patched)
> > 
> >
> > Unnecessary namespaces, as `Registry` is in scope here. Here and in the 
> > implementation below.

This _is_ needed due to ADL -- as `ResourceProviderManagerProcess` is defined 
in `mesos::internal`, just `Registry` would resolve to 
`mesos::internal::Registry` here. Dropping.


> On April 10, 2018, 3:05 p.m., Jan Schlicht wrote:
> > src/resource_provider/registrar.cpp
> > Lines 97 (patched)
> > 
> >
> > `Registry` is in scope here and used without the namespace in other 
> > parts of this unit. Let's be consistent and drop the namespace here and 
> > below.

See above. Dropping.


- Benjamin


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


On April 10, 2018, 2:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> ---
> 
> (Updated April 10, 2018, 2:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adjusts the control flow of the resource provider manager
> so that we can in the future make use of persisted resource provider
> information.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 9eb49f1327a0b598c5464a29a09ee286d7018f09 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66311/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66528: Added logging of failed resource provider registry updates.

[Benjamin Bannier]

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

(Updated April 10, 2018, 5:05 p.m.)


Review request for mesos and Jan Schlicht.


Changes
---

Addressed issues raised by Jan.


Repository: mesos


Description
---

Added logging of failed resource provider registry updates.


Diffs (updated)
-

  src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09 


Diff: https://reviews.apache.org/r/66528/diff/2/

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 63368: Added MemoryProfiler class to Libprocess.

[Benno Evers]

> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/include/process/memory_profiler.hpp
> > Lines 104-113 (patched)
> > 
> >
> > Does this class have to be nested? How about making it 
> > `jemalloc::State` in the same file?

None of them technically *need* to be nested, but it feels a bit cleaner to not 
litter the `process`-namespace unnecessarily. It also seems more consistent to 
me to treat all three classes as similar as possible.


> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/include/process/memory_profiler.hpp
> > Lines 117-120 (patched)
> > 
> >
> > Can you please mention here that it is safe to cache `MemoryProfiler` 
> > because it is destructed on process termination? Or, if you prefer to 
> > capture a lambda, to warn that users must guarantee that it is safe to 
> > invoke the lambda at a future point of time?

I'm dropping this as I'm now explicitly passing a pointer, but the reason it 
was safe was actually another one: `ProfilingRun` was a member of 
`MemoryProfiler`, so it will be destroyed before the base class, no matter 
whether it's global or not.


> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/include/process/memory_profiler.hpp
> > Lines 120-121 (patched)
> > 
> >
> > s/class/struct/
> > rm public:

Dropping this because it violates our style guide 
(https://google.github.io/styleguide/cppguide.html#Structs_vs._Classes)


> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/CMakeLists.txt
> > Lines 47 (patched)
> > 
> >
> > Please add *.hpp as well.

I think if we do this, it should be part of a separate patch series - right 
now, none of the public headers are included in the `CMakeLists.txt`.


> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 794 (patched)
> > 
> >
> > So if there is an error reading the jemalloc setting, the binary 
> > crashes?

No, only if the setting is read successfully and it is still active.


> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/memory_profiler.cpp
> > Lines 797 (patched)
> > 
> >
> > If we come here via `/stop` endpoint, can the still running timer 
> > occasionally stop the next profiling run?

Urgh, I'm not sure how I did forget about this one again, especially since the 
original motivation for introducing `ProfilingRun` was to avoid exactly this 
race :D


> On April 6, 2018, 2:02 p.m., Alexander Rukletsov wrote:
> > 3rdparty/libprocess/src/process.cpp
> > Lines 256 (patched)
> > 
> >
> > Let's make it `false` by default for now since it is an experimental 
> > feature, and turn it on after some time, ideally after some production 
> > testing together with feature graduation.

I'm not sure, keep in mind that the actual "feature" is hidden anyways behind 
the `--enable-jemalloc-allocator` configuration setting, which is off by 
default.

Without this, and assuming the user doesn't manually link against jemalloc, all 
endpoints will just return an error message saying that jemalloc couldn't be 
detected.


- Benno


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


On April 10, 2018, 2:39 p.m., Benno Evers wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63368/
> ---
> 
> (Updated April 10, 2018, 2:39 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class exposes profiling functionality of jemalloc memory allocator
> when it is detected to be the memory allocator of the current process.
> 
> In particular, it gives developers an easy method to collect and
> access heap profiles which report which pieces of code were
> responsible for allocating memory.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c895d3ac7b9cc5ffd6c8b57ff24def866eb0213d 
>   3rdparty/libprocess/include/Makefile.am 
> cd2c3bc62df8de5b50ec2fa830b3e2634ba11e28 
>   3rdparty/libprocess/include/process/memory_profiler.hpp 

Re: Review Request 63368: Added MemoryProfiler class to Libprocess.

[Benno Evers]

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

(Updated April 10, 2018, 2:39 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Address comments.


Repository: mesos


Description
---

This class exposes profiling functionality of jemalloc memory allocator
when it is detected to be the memory allocator of the current process.

In particular, it gives developers an easy method to collect and
access heap profiles which report which pieces of code were
responsible for allocating memory.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c895d3ac7b9cc5ffd6c8b57ff24def866eb0213d 
  3rdparty/libprocess/include/Makefile.am 
cd2c3bc62df8de5b50ec2fa830b3e2634ba11e28 
  3rdparty/libprocess/include/process/memory_profiler.hpp PRE-CREATION 
  3rdparty/libprocess/include/process/process.hpp 
c36df991b6a2c120ab0562e8ff907f9fbf8630d1 
  3rdparty/libprocess/src/CMakeLists.txt 
0ce7dac5deea94623530820d173ce3ffe5b42ea4 
  3rdparty/libprocess/src/memory_profiler.cpp PRE-CREATION 
  3rdparty/libprocess/src/process.cpp 9eb37465cd86f408d69f5f98fb76c4f4b93b9acd 


Diff: https://reviews.apache.org/r/63368/diff/9/

Changes: https://reviews.apache.org/r/63368/diff/8-9/


Testing
---


Thanks,

Benno Evers

Re: Review Request 63366: Add jemalloc release tarball and build rules.

[Benno Evers]

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

(Updated April 10, 2018, 2:38 p.m.)


Review request for mesos, Alexander Rukletsov and Benjamin Mahler.


Changes
---

Make jemalloc linux-only.


Repository: mesos


Description
---

Add jemalloc release tarball and build rules.


Diffs (updated)
-

  3rdparty/CMakeLists.txt 488e906486a583e74faceb44d906cee5036a8b99 
  3rdparty/Makefile.am 10b29c9e2d10073a817273c7038226d0ed4fd6ad 
  3rdparty/cmake/Versions.cmake 93f0322c1ac926bcfdcd4c1cfd9ba9f22bcf7099 
  3rdparty/jemalloc-5.0.1.tar.bz2 PRE-CREATION 
  3rdparty/versions.am 1bc87bb1ec2e74ebb2072f63163e3c1e8b4aad00 
  cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
  configure.ac f0f901f2e565352c2804cae7b2ac255da81ce45d 
  src/Makefile.am 9f4b6d369a23af337e2384e52e3e41f4017df38a 
  src/master/CMakeLists.txt b8953bd576586ac8548a1b51cf0a87bc8e9da4fc 
  src/slave/CMakeLists.txt 943e8f523f867e3dd4030f78eca7830500578faf 


Diff: https://reviews.apache.org/r/63366/diff/6/

Changes: https://reviews.apache.org/r/63366/diff/5-6/


Testing
---


Thanks,

Benno Evers

Re: Review Request 66528: Added logging of failed resource provider registry updates.

[Jan Schlicht]

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


Fix it, then Ship it!





src/resource_provider/registrar.cpp
Lines 288 (patched)


Let's use `WARNING` instead of `INFO` here.


- Jan Schlicht


On April 10, 2018, 2:56 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66528/
> ---
> 
> (Updated April 10, 2018, 2:56 p.m.)
> 
> 
> Review request for mesos and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added logging of failed resource provider registry updates.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.cpp 
> 9eb49f1327a0b598c5464a29a09ee286d7018f09 
> 
> 
> Diff: https://reviews.apache.org/r/66528/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66527: Avoided copy in accessing resource provider subscription field.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On April 10, 2018, 2:56 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66527/
> ---
> 
> (Updated April 10, 2018, 2:56 p.m.)
> 
> 
> Review request for mesos and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Avoided copy in accessing resource provider subscription field.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
> 
> 
> Diff: https://reviews.apache.org/r/66527/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66508: Made sure test agent has reached stable state before starting test.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On April 9, 2018, 4:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66508/
> ---
> 
> (Updated April 9, 2018, 4:07 p.m.)
> 
> 
> Review request for mesos, Chun-Hung Hsiao and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We require a certain number of offers in
> `SlaveTest.ResourceProviderPublishAll`, so we need to make sure that
> the agent knows all resources available during the test before offers
> are being generated.
> 
> In this patch we await all `UpdateSlaveMessage` from the agent to make
> sure the agent has seen all resources. Only then do we register a
> framework. This greatly minimizes the risk of not seeing the expected
> number of offers which now should only happen if due to
> master-internal message reordering the allocator sees the framework
> before it sees the latest agent resource update.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 646a2b9e046fb37dbe0610ea0848b50192913deb 
> 
> 
> Diff: https://reviews.apache.org/r/66508/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [66493]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose --disable-libtool-wrappers' ENVIRONMENT='GLOG_v=1 
MESOS_VERBOSE=1'; ./support/docker-build.sh

- Mesos Reviewbot


On April 10, 2018, 6:36 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 10, 2018, 6:36 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8761
> https://issues.apache.org/jira/browse/MESOS-8761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made FreeBSD default to non-GNU ld.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
> 
> 
> Diff: https://reviews.apache.org/r/66493/diff/2/
> 
> 
> Testing
> ---
> 
> make on FreeBSD, with both lld and gold.
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 66311: Implement recovery of resource provider manager.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66311 was successfully built and tested.

Reviews applied: `['66508', '66308', '66309', '66526', '66310', '66311']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66311

- Mesos Reviewbot Windows


On April 10, 2018, 2:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> ---
> 
> (Updated April 10, 2018, 2:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adjusts the control flow of the resource provider manager
> so that we can in the future make use of persisted resource provider
> information.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 9eb49f1327a0b598c5464a29a09ee286d7018f09 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66311/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 66311: Implement recovery of resource provider manager.

[Jan Schlicht]

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




src/resource_provider/manager.cpp
Lines 65 (patched)


Blank line prior to this one.



src/resource_provider/manager.cpp
Lines 201 (patched)


Unnecessary namespaces, as `Registry` is in scope here. Here and in the 
implementation below.



src/resource_provider/manager.cpp
Lines 237 (patched)


`registry` as name is a bit misleading, how about `recovered`?



src/resource_provider/manager.cpp
Lines 240 (patched)


s/recovery/recover



src/resource_provider/registrar.cpp
Lines 97 (patched)


`Registry` is in scope here and used without the namespace in other parts 
of this unit. Let's be consistent and drop the namespace here and below.


- Jan Schlicht


On April 10, 2018, 2:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66311/
> ---
> 
> (Updated April 10, 2018, 2:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adjusts the control flow of the resource provider manager
> so that we can in the future make use of persisted resource provider
> information.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 9eb49f1327a0b598c5464a29a09ee286d7018f09 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66311/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 66527: Avoided copy in accessing resource provider subscription field.

[Benjamin Bannier]

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

Review request for mesos and Jan Schlicht.


Repository: mesos


Description
---

Avoided copy in accessing resource provider subscription field.


Diffs
-

  src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 


Diff: https://reviews.apache.org/r/66527/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 66528: Added logging of failed resource provider registry updates.

[Benjamin Bannier]

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

Review request for mesos and Jan Schlicht.


Repository: mesos


Description
---

Added logging of failed resource provider registry updates.


Diffs
-

  src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09 


Diff: https://reviews.apache.org/r/66528/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66310: Passed on registrar when constructing resource provider manager.

[Benjamin Bannier]

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

(Updated April 10, 2018, 2:07 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Rebased.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

In order to support recovering resource provider manager information
in the future, we need to adjust the construction of the manager to be
able to consume a registrar.

This patch lays the groundwork by adjusting interfaces and their
usage; we will make use of the passed on information in a following
patch.


Diffs (updated)
-

  src/resource_provider/manager.hpp bc017fa3998b780cec82718cabe2a8b470a66db2 
  src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66310/diff/2/

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66311: Implement recovery of resource provider manager.

[Benjamin Bannier]

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

(Updated April 10, 2018, 2:07 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Rebased.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

This patch adjusts the control flow of the resource provider manager
so that we can in the future make use of persisted resource provider
information.


Diffs (updated)
-

  src/resource_provider/manager.cpp 2d3ffcdd01671f81729fe538348f2ca4e555eb4f 
  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66311/diff/3/

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66309: Externalize creation of resource provider manager backing storage.

[Benjamin Bannier]

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

(Updated April 10, 2018, 2:07 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Addressed issues raised by Jan.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

This patch changes the way the storage backing an agent's resource
provider registrar is created: while before we created it implicitly
when constructing the registrar, we now consume storage passed on
construction.

Being able to explicitly inject the used storage simplifies testing.


Diffs (updated)
-

  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66309/diff/2/

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66308: Delayed construction of the agent's resource provider manager.

[Benjamin Bannier]

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

(Updated April 10, 2018, 2:07 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Rebased.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

By delaying the construction of the agent's resource provider manager
we prepare for a following patch introducing a dependency on the
resource provider manager on the agent's ID.

Depending on whether the agent was able to recover an agent ID from
its log or still needs to obtain on in a first registration with the
master, we can only construct the resource provider manager after
different points in the initialization of the agent. To capture the
common code we introduce a helper function performing the necessary
steps.


Diffs (updated)
-

  src/slave/slave.hpp d00c7b2e2466fd1de71e9e55343a27d3808bde3e 
  src/slave/slave.cpp e5d6c3fac5054a6b0a0b5b77abd850a35be6ccc5 
  src/tests/resource_provider_manager_tests.cpp 
c52541bf130ccf4795b989b53331176a64a097ea 


Diff: https://reviews.apache.org/r/66308/diff/3/

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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66309: Externalize creation of resource provider manager backing storage.

[Benjamin Bannier]

> On March 28, 2018, 3:30 p.m., Jan Schlicht wrote:
> > src/resource_provider/registrar.hpp
> > Lines 68-69 (original), 70-71 (patched)
> > 
> >
> > I don't like the semantics of this:
> > A `state::Storage` doesn't provide any information if it belongs to 
> > master or agent but we instantiate an `AgentRegistrar` anyways.
> > 
> > Hence, please rename `AgentRegistrar` to something more general that 
> > indicates that this isn't tied to an agent state.

That makes much sense. I have added a follow up patch to address this to not 
put too many changes into this one, https://reviews.apache.org/r/66526/.


- Benjamin


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


On April 10, 2018, 2:07 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66309/
> ---
> 
> (Updated April 10, 2018, 2:07 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-8735
> https://issues.apache.org/jira/browse/MESOS-8735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the way the storage backing an agent's resource
> provider registrar is created: while before we created it implicitly
> when constructing the registrar, we now consume storage passed on
> construction.
> 
> Being able to explicitly inject the used storage simplifies testing.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/registrar.hpp 
> 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
>   src/resource_provider/registrar.cpp 
> 9eb49f1327a0b598c5464a29a09ee286d7018f09 
>   src/tests/resource_provider_manager_tests.cpp 
> c52541bf130ccf4795b989b53331176a64a097ea 
> 
> 
> Diff: https://reviews.apache.org/r/66309/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 66526: Renamed resource provider `AgentRegistrar` to `GenericRegistrar`.

[Benjamin Bannier]

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

Review request for mesos, Chun-Hung Hsiao, Greg Mann, and Jan Schlicht.


Bugs: MESOS-8735
https://issues.apache.org/jira/browse/MESOS-8735


Repository: mesos


Description
---

This registrar and its matching process can work on generic storage
and is currently used to work on an agent's persist store.

Its name should not be tied the agent, so we rename the registrar in
this patch.


Diffs
-

  src/resource_provider/registrar.hpp 39f45b0a2a7c35bfe72a9305f5248409e4a3ef45 
  src/resource_provider/registrar.cpp 9eb49f1327a0b598c5464a29a09ee286d7018f09 


Diff: https://reviews.apache.org/r/66526/diff/1/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 66415: Changed SLEEP_COMMAND to use ping instead of powershell Start-Sleep on Windows.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66415 was successfully built and tested.

Reviews applied: `['66415']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66415

- Mesos Reviewbot Windows


On April 5, 2018, 8:51 p.m., Eric Mumau wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66415/
> ---
> 
> (Updated April 5, 2018, 8:51 p.m.)
> 
> 
> Review request for Akash Gupta, Andrew Schwartzmeyer and John Kordich.
> 
> 
> Bugs: MESOS-8308
> https://issues.apache.org/jira/browse/MESOS-8308
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed SLEEP_COMMAND to use ping instead of powershell Start-Sleep on 
> Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gtest.hpp 
> a8ae3d90519aae7788e0874a22776cff682173ba 
> 
> 
> Diff: https://reviews.apache.org/r/66415/diff/2/
> 
> 
> Testing
> ---
> 
> Tested on 2 Windows platforms.
> 
> 
> Thanks,
> 
> Eric Mumau
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[Mesos Reviewbot Windows]

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



PASS: Mesos patch 66493 was successfully built and tested.

Reviews applied: `['66493']`

All the build artifacts available at: 
http://dcos-win.westus.cloudapp.azure.com/mesos-build/review/66493

- Mesos Reviewbot Windows


On April 10, 2018, 6:36 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 10, 2018, 6:36 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8761
> https://issues.apache.org/jira/browse/MESOS-8761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made FreeBSD default to non-GNU ld.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
> 
> 
> Diff: https://reviews.apache.org/r/66493/diff/2/
> 
> 
> Testing
> ---
> 
> make on FreeBSD, with both lld and gold.
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[David Forsythe]

> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 308-311 (patched)
> > 
> >
> > I was briefly wondering why we checked for clang below. The reason is 
> > of course that cmake by default uses the compiler as linker, and there is 
> > an incompatibility on FreeBSD when compiling with clang and linking with 
> > `ld.bfd`.
> > 
> > I think it would be useful to describe this in more detail here so we 
> > have a chance to understand when this check can go in the future. Ideally 
> > we'd be able to link some upstream issue, but it may not exist.
> 
> Andrew Schwartzmeyer wrote:
> Yeah, it took me a while to reason through this too, even with your 
> comment Benjamin. It's because of how the linker command is formed (described 
> [here](https://cmake.org/pipermail/cmake/2014-August/058268.html)). Not 
> knowing this will leave you bewildered here.
> 
> David Forsythe wrote:
> I'll make the comment a bit more descriptive.

I'm the beginning to think this isn't actually a FreeBSD specific issue, but I 
left the comment FreeBSD specific (and left the CMAKE_SYSTEM_NAME check) since 
I haven't had anyone tell me they've reproduced it on Linux.


> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 326-329 (patched)
> > 
> >
> > I am not sure this is an issue, but customary there are similar 
> > variables defined for other build types like `DEBUG`, `RELEASE`, etc. 
> > (e.g., `CMAKE_EXE_LINKER_FLAGS_DEBUG`).
> > 
> > Do we have something to set all related flags @andschwa?
> 
> Andrew Schwartzmeyer wrote:
> It might soon be time to put this into a function, but yeah, it'd be a 
> nested for loop akin to this:
> 
> ```
> foreach (lang C CXX)
>   list(APPEND CMAKE_${lang}_FORWARD_ARGS
> ${CMAKE_FORWARD_ARGS}
> -DCMAKE_${lang}_COMPILER=${CMAKE_${lang}_COMPILER}
> -DCMAKE_${lang}_COMPILER_LAUNCHER=${CMAKE_${lang}_COMPILER_LAUNCHER}
> -DCMAKE_${lang}_FLAGS=${CMAKE_${lang}_FLAGS})
> 
>   foreach (config DEBUG RELEASE RELWITHDEBINFO MINSIZEREL)
> list(APPEND CMAKE_${lang}_FORWARD_ARGS
>   -DCMAKE_${lang}_FLAGS_${config}=${CMAKE_${lang}_FLAGS_${config}})
>   endforeach ()
> endforeach ()
> 
> ```
> 
> Note that this only matters if you're using a multi-configuration 
> generator (like Visual Studio and some other IDEs), but not `make` nor 
> `ninja`.
> 
> David Forsythe wrote:
> Will switch over to something like that.

I left off the the config loop because it didn't seem like the other flags 
being set in this file were using them. It wasn't clear to me if they were 
actually needed, so if I should add them just let me know.


- David


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


On April 10, 2018, 6:36 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 10, 2018, 6:36 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
> Bugs: MESOS-8761
> https://issues.apache.org/jira/browse/MESOS-8761
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made FreeBSD default to non-GNU ld.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 
> 
> 
> Diff: https://reviews.apache.org/r/66493/diff/2/
> 
> 
> Testing
> ---
> 
> make on FreeBSD, with both lld and gold.
> 
> 
> Thanks,
> 
> David Forsythe
> 
>

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[David Forsythe]

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

(Updated April 10, 2018, 6:36 a.m.)


Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.


Bugs: MESOS-8761
https://issues.apache.org/jira/browse/MESOS-8761


Repository: mesos


Description
---

Made FreeBSD default to non-GNU ld.


Diffs (updated)
-

  cmake/CompilationConfigure.cmake 3cb072ddcd286c0e40d44eaeba210ddf1796975c 


Diff: https://reviews.apache.org/r/66493/diff/2/

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


Testing
---

make on FreeBSD, with both lld and gold.


Thanks,

David Forsythe

Re: Review Request 66493: Made FreeBSD default to non-GNU ld.

[David Forsythe]

> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 308-311 (patched)
> > 
> >
> > I was briefly wondering why we checked for clang below. The reason is 
> > of course that cmake by default uses the compiler as linker, and there is 
> > an incompatibility on FreeBSD when compiling with clang and linking with 
> > `ld.bfd`.
> > 
> > I think it would be useful to describe this in more detail here so we 
> > have a chance to understand when this check can go in the future. Ideally 
> > we'd be able to link some upstream issue, but it may not exist.
> 
> Andrew Schwartzmeyer wrote:
> Yeah, it took me a while to reason through this too, even with your 
> comment Benjamin. It's because of how the linker command is formed (described 
> [here](https://cmake.org/pipermail/cmake/2014-August/058268.html)). Not 
> knowing this will leave you bewildered here.

I'll make the comment a bit more descriptive.


> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 315 (patched)
> > 
> >
> > This conditional seems not needed as cmake will automatically detect if 
> > `LD_PROGRAM` was set by the user. Is this a variable we should document as 
> > a customization point?
> 
> Andrew Schwartzmeyer wrote:
> Do you mean documenting like as an `option(...)` and in the docs, or just 
> in the docs? Maybe we should wait until the FreeBSD build is working, and 
> then add a doc for it (in which we could also explain the weirdness with 
> linkers).

Hm, I didn't realize find_program wouldn't run if I set this on my own. I'll 
clean it up.

As for documenting it, I figured that it would be best to wait until the build 
was fixed and settled. Even if someone is building on FreeBSD at this point 
nothing is really working. It might be a bit pre-mature to start surfacing docs 
about the build (which probably means there should be a ticket about 
documenting this?).


> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 318 (patched)
> > 
> >
> > Since this variable will be setable from the outside _Alternative_ 
> > seems redundant here.

Removed.


> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 322 (patched)
> > 
> >
> > Could we make clearer what a user needs to do to have this check pass? 
> > The error message appears too technical to me.
> 
> Andrew Schwartzmeyer wrote:
> +1 I have no clue what I would do to get a "non-base LD."

This isn't even accurate since lld is in base. I'll update it.


> On April 9, 2018, 1:13 p.m., Benjamin Bannier wrote:
> > cmake/CompilationConfigure.cmake
> > Lines 326-329 (patched)
> > 
> >
> > I am not sure this is an issue, but customary there are similar 
> > variables defined for other build types like `DEBUG`, `RELEASE`, etc. 
> > (e.g., `CMAKE_EXE_LINKER_FLAGS_DEBUG`).
> > 
> > Do we have something to set all related flags @andschwa?
> 
> Andrew Schwartzmeyer wrote:
> It might soon be time to put this into a function, but yeah, it'd be a 
> nested for loop akin to this:
> 
> ```
> foreach (lang C CXX)
>   list(APPEND CMAKE_${lang}_FORWARD_ARGS
> ${CMAKE_FORWARD_ARGS}
> -DCMAKE_${lang}_COMPILER=${CMAKE_${lang}_COMPILER}
> -DCMAKE_${lang}_COMPILER_LAUNCHER=${CMAKE_${lang}_COMPILER_LAUNCHER}
> -DCMAKE_${lang}_FLAGS=${CMAKE_${lang}_FLAGS})
> 
>   foreach (config DEBUG RELEASE RELWITHDEBINFO MINSIZEREL)
> list(APPEND CMAKE_${lang}_FORWARD_ARGS
>   -DCMAKE_${lang}_FLAGS_${config}=${CMAKE_${lang}_FLAGS_${config}})
>   endforeach ()
> endforeach ()
> 
> ```
> 
> Note that this only matters if you're using a multi-configuration 
> generator (like Visual Studio and some other IDEs), but not `make` nor 
> `ninja`.

Will switch over to something like that.


- David


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


On April 10, 2018, 6:36 a.m., David Forsythe wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66493/
> ---
> 
> (Updated April 10, 2018, 6:36 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Benjamin Bannier.
> 
> 
>