Re: Review Request 68813: Added support for `Option` / `Option`.

[Michael Park]

n.

I think one thing to consider is what we want `Option` to be.
Is it a temporary stand-in for `std::optional` until it can be replaced?
or are we unsatisfied with the `std::optional` design and we want `Option`
to actually be a different thing? IIRC, even for `Option`, assignment
semantics are deviate from `std::optional`.

---

As for the standard, we __need__ to answer questions around what equality
and assignment means for references. We don't have to here...

---

Regarding the lifetime concerns,

```
hashmap fun();
Option value = fun().get(key);
```

This is unsafe regardless of whether there's an `Option` there or not.
Even if we were to have used `const V&` with `at`, that `hashmap`
falls on its face and we have a dangling reference.

So it can't be on `Option` to solve that problem.

---

I've already mentioned this to bmahler, but as far as return types are
concerned, I would personally just opt for `T*` rather than `Option`.

```
// Now, I'm not assuming the key is present, so naturally,
// I get an optional reference instead of the reference:
T* value = hashmap.get(key);

if (value) {
  value->foo();
}
```

Considering only this use case (not worrying about stuff like comparison
semantics since it's not our primary use case at least at the moment),
the only change is `value.isSome()` vs `value` as the `if` condition.

---

To me the benefit of `Option` is as a function parameter, since taking
a `T*` there doesn't allow passing temporaries (which is safe in a function
call context) and passing a `T` to `const Option&` incurs a copy.

If we want this use case of passing a temporary, we cannot delete
the rvalue reference ctor.


- Michael


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


On Sept. 22, 2018, 6:09 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68813/
> ---
> 
> (Updated Sept. 22, 2018, 6:09 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benno Evers, and Michael Park.
> 
> 
> Bugs: MESOS-9252
> https://issues.apache.org/jira/browse/MESOS-9252
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds support for options of references. While this is still
> under debate for `std::optional`, there are some use cases in
> stout that can benefit from this:
> 
>   // None if the value is not found, otherwise a reference
>   // to the value.
>   Option t = hashmap.get("key");
> 
> Assignment and equality are deleted in order to avoid confusion
> around which of the 2 possible behaviors they provide (e.g. are
> the references being compared? or are the objects being referred
> to being compared?)
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/option.hpp 
> 8feed012a55fed6eab89c883958324f3345e46e9 
> 
> 
> Diff: https://reviews.apache.org/r/68813/diff/1/
> 
> 
> Testing
> ---
> 
> Test added in subsequent patch.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 68820: Bumped mesos-tidy to upstream release_70.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Sept. 23, 2018, 12:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68820/
> ---
> 
> (Updated Sept. 23, 2018, 12:32 p.m.)
> 
> 
> Review request for mesos, Michael Park and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Bumped mesos-tidy to upstream release_70.
> 
> 
> Diffs
> -
> 
>   support/clang-tidy 188d71f1675241af771a092e6bdb1aff9525a4f6 
>   support/mesos-tidy/Dockerfile ea28e40999add5218ab0a101e8b75ef78487b8a6 
> 
> 
> Diff: https://reviews.apache.org/r/68820/diff/1/
> 
> 
> Testing
> ---
> 
> * `make check` in upstream tree
> * confirmed that new image does not introduce new reports we can fix
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Michael Park]

> On July 20, 2018, 2:29 p.m., Michael Park wrote:
> > Looks good!
> > 
> > It might be worth considering using the `OStreamWrapper` stuff for the 
> > `ostream` API.
> > I know writing to `StringBuffer` is faster than writing to 
> > `OStreamWrapper`, but
> > I think just writing to `OStreamWrapper` would be faster than writing to 
> > `StringBuffer`
> > then copying that into a `std::string` then writing that to `ostream` in 
> > the end anyway.
> 
> Benjamin Mahler wrote:
> > I think just writing to OStreamWrapper would be faster than writing to 
> StringBuffer
> 
> That had seemed doubtful to me based on their documentation:
> http://rapidjson.org/md_doc_stream.html#iostreamWrapper
> 
> I would be interesting to get the numbers! However, we don't write to 
> ostream in the end any longer:
> https://reviews.apache.org/r/67992/
> 
> I could remove the `<<` operator but I figured I would just leave it 
> untouched for now.

Yeah, you left out the second half of my sentence but I assume it wasn't 
intentional.
I didn't notice the patch to replace `ostringstream`. Sounds good to me!


> On July 20, 2018, 2:29 p.m., Michael Park wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp
> > Line 137 (original), 95 (patched)
> > <https://reviews.apache.org/r/67988/diff/1/?file=2061761#file2061761line158>
> >
> > Looks like `GetString` returns a `const char*`. We should provide the 
> > length here: `{buffer.GetString(), buffer.GetSize()}`.
> 
> Benjamin Mahler wrote:
> Ah nice catch! This should avoid walking the string right? I'll re-run 
> the numbers.

Yep, exactly.


- Michael


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


On July 19, 2018, 8:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67988/
> ---
> 
> (Updated July 19, 2018, 8:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and Michael Park.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reduces the time needed for the client to finish receiving a
> master's /state response by 50% in the `StateQuery` benchmark:
> 
> minq1q3   max
> baseline   6.52  6.76  7.33  8.26
> rapidjson w/ SIMD  3.48  3.54  4.12  4.4
> rapidjson  3.29  3.32  3.65  3.85
> 
> SIMD is left disabled for now since it showed slightly slower
> results.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 2314980e185ee61cc2ea54f1b4d2a8b35e58121c 
> 
> 
> Diff: https://reviews.apache.org/r/67988/diff/2/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Michael Park]

> On July 20, 2018, 3:58 a.m., Benno Evers wrote:
> > 3rdparty/stout/include/stout/jsonify.hpp
> > Line 370 (original), 278 (patched)
> > <https://reviews.apache.org/r/67988/diff/1/?file=2061761#file2061761line403>
> >
> > If we care enough about performance to make a templatized overload for 
> > `const char(&)[N]`, maybe we should also add one for `std::string&&`?

The `const char (&)[N]` exists to avoid constructing a temporary
`std::string` from a string literal argument at the moment.
What cases would adding `std::string&&` improve? We can't exactly
move the `string` anyway right?


- Michael


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


On July 19, 2018, 8:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67988/
> ---
> 
> (Updated July 19, 2018, 8:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and Michael Park.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reduces the time needed for the client to finish receiving a
> master's /state response by 50% in the `StateQuery` benchmark:
> 
> minq1q3   max
> baseline   6.52  6.76  7.33  8.26
> rapidjson w/ SIMD  3.48  3.54  4.12  4.4
> rapidjson  3.29  3.32  3.65  3.85
> 
> SIMD is left disabled for now since it showed slightly slower
> results.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 2314980e185ee61cc2ea54f1b4d2a8b35e58121c 
> 
> 
> Diff: https://reviews.apache.org/r/67988/diff/1/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67988: Improved performance of jsonify by integrating with rapidjson.

[Michael Park]

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


Fix it, then Ship it!




Looks good!

It might be worth considering using the `OStreamWrapper` stuff for the 
`ostream` API.
I know writing to `StringBuffer` is faster than writing to `OStreamWrapper`, but
I think just writing to `OStreamWrapper` would be faster than writing to 
`StringBuffer`
then copying that into a `std::string` then writing that to `ostream` in the 
end anyway.


3rdparty/stout/include/stout/jsonify.hpp
Line 137 (original), 95 (patched)
<https://reviews.apache.org/r/67988/#comment289186>

Looks like `GetString` returns a `const char*`. We should provide the 
length here: `{buffer.GetString(), buffer.GetSize()}`.


- Michael Park


On July 19, 2018, 8:38 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67988/
> ---
> 
> (Updated July 19, 2018, 8:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benno Evers, and Michael Park.
> 
> 
> Bugs: MESOS-9092
> https://issues.apache.org/jira/browse/MESOS-9092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reduces the time needed for the client to finish receiving a
> master's /state response by 50% in the `StateQuery` benchmark:
> 
> minq1q3   max
> baseline   6.52  6.76  7.33  8.26
> rapidjson w/ SIMD  3.48  3.54  4.12  4.4
> rapidjson  3.29  3.32  3.65  3.85
> 
> SIMD is left disabled for now since it showed slightly slower
> results.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 2314980e185ee61cc2ea54f1b4d2a8b35e58121c 
> 
> 
> Diff: https://reviews.apache.org/r/67988/diff/1/
> 
> 
> Testing
> ---
> 
> Tested at the end of this chain, since this is split across 
> stout/libprocess/mesos.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67861: Updated json parsing to avoid copying between picojson and stout JSON.

[Michael Park]

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




3rdparty/stout/include/stout/json.hpp
Lines 343-347 (original), 352-377 (patched)
<https://reviews.apache.org/r/67861/#comment288785>

While we're here, we could also replace these with the pointer versions of 
`boost::get`. We might get some better performance: https://godbolt.org/g/B84cLb


- Michael Park


On July 9, 2018, 6:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67861/
> ---
> 
> (Updated July 9, 2018, 6:40 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When looking at some profiling data, it looks as though we approximately
> double the cost of JSON parsing due to having to convert from
> `picojson::value` to `JSON::Value`.
> 
> Michael Park pointed me to the parsing "context" that's customizable in
> picojson. This patch replaces our conversion with a parsing context in
> order to parse directly into JSON::Value and avoid copying.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> c374e29829e4ea02ab3a8494b6ab28c166579516 
> 
> 
> Diff: https://reviews.apache.org/r/67861/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67861: Updated json parsing to avoid copying between picojson and stout JSON.

[Michael Park]

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




3rdparty/stout/include/stout/json.hpp
Line 855 (original), 892 (patched)
<https://reviews.apache.org/r/67861/#comment288784>

Perhaps leave a small comment + a pointer to 
https://github.com/kazuho/picojson/blob/v1.3.0/picojson.h#L820-L870 here.


- Michael Park


On July 9, 2018, 6:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67861/
> ---
> 
> (Updated July 9, 2018, 6:40 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When looking at some profiling data, it looks as though we approximately
> double the cost of JSON parsing due to having to convert from
> `picojson::value` to `JSON::Value`.
> 
> Michael Park pointed me to the parsing "context" that's customizable in
> picojson. This patch replaces our conversion with a parsing context in
> order to parse directly into JSON::Value and avoid copying.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> c374e29829e4ea02ab3a8494b6ab28c166579516 
> 
> 
> Diff: https://reviews.apache.org/r/67861/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 67861: Updated json parsing to avoid copying between picojson and stout JSON.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On July 9, 2018, 6:40 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/67861/
> ---
> 
> (Updated July 9, 2018, 6:40 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When looking at some profiling data, it looks as though we approximately
> double the cost of JSON parsing due to having to convert from
> `picojson::value` to `JSON::Value`.
> 
> Michael Park pointed me to the parsing "context" that's customizable in
> picojson. This patch replaces our conversion with a parsing context in
> order to parse directly into JSON::Value and avoid copying.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> c374e29829e4ea02ab3a8494b6ab28c166579516 
> 
> 
> Diff: https://reviews.apache.org/r/67861/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 66114: Added supporting scripts for Mesos LLVM Tools.

[Michael Park]

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

Review request for mesos, Benjamin Bannier and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/llvm/README.md PRE-CREATION 
  support/llvm/install.sh PRE-CREATION 


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


Testing
---


Thanks,

Michael Park

Review Request 66115: Added `mesos-tidy.py`, a modified version of `run-clang-tidy.py`.

[Michael Park]

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

Review request for mesos, Benjamin Bannier and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/mesos-tidy.py PRE-CREATION 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 66007: CMake: Set C++11 as standard automatically.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 9, 2018, 2:37 p.m., Andrew Schwartzmeyer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66007/
> ---
> 
> (Updated March 9, 2018, 2:37 p.m.)
> 
> 
> Review request for mesos, Akash Gupta, Benjamin Bannier, Jeff Coffler, John 
> Kordich, Joseph Wu, and Michael Park.
> 
> 
> Bugs: MESOS-8033
> https://issues.apache.org/jira/browse/MESOS-8033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of setting the compiler option manually, we use the
> `CMAKE_CXX_STANDARD` variable to set the default for all targets. This
> automatically appends the correct flag for each compiler.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake efee36c1ffda096a97af23d481fc0d0903124e54 
> 
> 
> Diff: https://reviews.apache.org/r/66007/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Andrew Schwartzmeyer
> 
>

Re: Review Request 66031: Avoided copying when possible in Option::getOrElse.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 12, 2018, 5:08 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/66031/
> ---
> 
> (Updated March 12, 2018, 5:08 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This mimics the behavior of std::optional::value_or.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/option.hpp 
> 72403e770afbb59bd1c58b621c2bc566de3cd979 
> 
> 
> Diff: https://reviews.apache.org/r/66031/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65823: Avoid copies of task reconciliation status updates.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 27, 2018, 3:32 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65823/
> ---
> 
> (Updated Feb. 27, 2018, 3:32 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp f2c2dbe6a4568027f11ad14121c2f9a1d1a43f80 
> 
> 
> Diff: https://reviews.apache.org/r/65823/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65683: Updated discard handling in 'Docker::inspect()'.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 27, 2018, 2:35 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65683/
> ---
> 
> (Updated Feb. 27, 2018, 2:35 a.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, Michael Park, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-8575
> https://issues.apache.org/jira/browse/MESOS-8575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated discard handling in 'Docker::inspect()'.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 0116ca8f28303905d80cd6b3a33a7a7feacc8f8a 
>   src/docker/docker.cpp f7a4466292fcedd7e97740698475da2c616ca1d0 
> 
> 
> Diff: https://reviews.apache.org/r/65683/diff/7/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65786: Prevented Docker library from terminating incorrect processes.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 23, 2018, 2:37 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65786/
> ---
> 
> (Updated Feb. 23, 2018, 2:37 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Gilbert Song, and Michael Park.
> 
> 
> Bugs: MESOS-8575
> https://issues.apache.org/jira/browse/MESOS-8575
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the Docker library might call `os::killtree()` on a
> PID after the associated subprocess had already terminated, which
> could lead to an unknown process being incorrectly killed.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 3df370e988fce12d323ff6b441da15dab27bdd28 
> 
> 
> Diff: https://reviews.apache.org/r/65786/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details are found later in this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 65514: Introduced a CHECK_NOTERROR macro.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/check.hpp
Lines 123 (patched)
<https://reviews.apache.org/r/65514/#comment278331>

Ideally `t.error()` here would always return the `E` and we can simply 
access the message via `t.error().message`, but it returns `string` when `E == 
Error` for backward-compatibility.

One approach here would be to do `string(t.error())` and add a `operator 
const string&()` to `Error`.


- Michael Park


On Feb. 5, 2018, 2:06 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65514/
> ---
> 
> (Updated Feb. 5, 2018, 2:06 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/check.hpp 
> 6a33579b6c7590590fd3157e14cdc72bcc2bc27b 
> 
> 
> Diff: https://reviews.apache.org/r/65514/diff/1/
> 
> 
> Testing
> ---
> 
> make check, used in a subsequent patch
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65515: Added rvalue reference Try::get overloads.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 5, 2018, 2:08 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65515/
> ---
> 
> (Updated Feb. 5, 2018, 2:08 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-2664
> https://issues.apache.org/jira/browse/MESOS-2664
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/try.hpp 
> 90f8aed56ce5d77e70af9e516faad152083d1488 
> 
> 
> Diff: https://reviews.apache.org/r/65515/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65515: Added rvalue reference Try::get overloads.

[Michael Park]

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




3rdparty/stout/include/stout/try.hpp
Lines 73-86 (original), 73-100 (patched)
<https://reviews.apache.org/r/65515/#comment277383>

It seems like MSVC is not happy with `const_cast<T&&>(non-lvalue)`.
These days, it's easier to use a templated helper to avoid the duplication.

```cpp
  T& get() & { return get(*this); }
  const T& get() const & { return get(*this); }
  T&& get() && { return get(std::move(*this)); }
  const T&& get() const && { return get(std::move(*this)); }
  
  template 
  static auto get(Self&& self) -> 
decltype(std::forward(self).data.get())
  {
if (!self.data.isSome()) {
  assert(self.error_.isSome());
  ABORT("Try::get() but state == ERROR: " + self.error_->message);
}
return std::forward(self).data.get();
  }
```


- Michael Park


On Feb. 5, 2018, 2:08 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65515/
> -------
> 
> (Updated Feb. 5, 2018, 2:08 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-2664
> https://issues.apache.org/jira/browse/MESOS-2664
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/try.hpp 
> 90f8aed56ce5d77e70af9e516faad152083d1488 
> 
> 
> Diff: https://reviews.apache.org/r/65515/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65512: Fixed race in Python egg caching.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Feb. 5, 2018, 11:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65512/
> ---
> 
> (Updated Feb. 5, 2018, 11:50 a.m.)
> 
> 
> Review request for mesos, Kevin Klues, Michael Park, and Vinod Kone.
> 
> 
> Bugs: MESOS-8546
> https://issues.apache.org/jira/browse/MESOS-8546
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Python modules containing native eggs commonly get cached within the
> PYTHON_EGG_CACHE folder when importing them. This caching however
> fails when being concurrently invoked for the same egg.
> We need our custom Python executor to be protected against concurrent
> egg caching. Setting the PYTHON_EGG_CACHE towards a MESOS_SANDBOX
> subfolder solves this.
> 
> 
> Diffs
> -
> 
>   src/examples/python/test-executor.in 
> 4c402cd317ebf657391c59340c37a7003130bdc1 
> 
> 
> Diff: https://reviews.apache.org/r/65512/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65464: Introduced `mesos-build` along with pre-built docker images.

[Michael Park]

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

(Updated Feb. 4, 2018, 12:22 p.m.)


Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

The layout of `mesos-build` is similar to `mesos-tidy`.

The `mesos-build.sh` will be the user-facing driver, and `buildbot.sh`
invokes `mesos-build.sh` along with the clean-up (`docker rmi`) phase.
The `mesos-build` directory contains docker files and a common
`entrypoint.sh`. The docker images are built with all of the
dependencies required, and the various testing configurations are set
within `entrypoint.sh`.


Diffs (updated)
-

  support/jenkins/buildbot.sh dab4b72b645eaca382a70acef4220fa04af96e36 
  support/mesos-build.sh PRE-CREATION 
  support/mesos-build/centos-7.dockerfile PRE-CREATION 
  support/mesos-build/enable-devtoolset-4.sh PRE-CREATION 
  support/mesos-build/entrypoint.sh PRE-CREATION 
  support/mesos-build/ubuntu-16.04.dockerfile PRE-CREATION 


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

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


Testing (updated)
---

https://builds.apache.org/job/Mesos-Buildbot-Test/103/


Thanks,

Michael Park

Re: Review Request 65464: Introduced `mesos-build` along with pre-built docker images.

[Michael Park]

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

(Updated Feb. 3, 2018, 5:14 p.m.)


Review request for mesos and Benjamin Bannier.


Changes
---

Fixed CentOS builds.


Repository: mesos


Description
---

The layout of `mesos-build` is similar to `mesos-tidy`.

The `mesos-build.sh` will be the user-facing driver, and `buildbot.sh`
invokes `mesos-build.sh` along with the clean-up (`docker rmi`) phase.
The `mesos-build` directory contains docker files and a common
`entrypoint.sh`. The docker images are built with all of the
dependencies required, and the various testing configurations are set
within `entrypoint.sh`.


Diffs (updated)
-

  support/jenkins/buildbot.sh dab4b72b645eaca382a70acef4220fa04af96e36 
  support/mesos-build.sh PRE-CREATION 
  support/mesos-build/centos-7.dockerfile PRE-CREATION 
  support/mesos-build/entrypoint.sh PRE-CREATION 
  support/mesos-build/ubuntu-16.04.dockerfile PRE-CREATION 


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

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


Testing (updated)
---

https://builds.apache.org/job/Mesos-Buildbot-Test/101/


Thanks,

Michael Park

Review Request 65463: Removed the `mesos-style.py` run from `buildbot.sh`.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

The Apache CI Jenkins instances have various Python configurations
which lead to `pip install --user virtualenv` failing due to old
`setuptools`. For now, we remove this from `buildbot.sh`. We'll
shortly perform this check inside the Docker container such that
we operate in a more stable environment.


Diffs
-

  support/jenkins/buildbot.sh 7f78509699b036df025c314fe913158f54402014 


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


Testing
---


Thanks,

Michael Park

Review Request 65464: WIP: Introduced `mesos-build`, along with pre-built docker images.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Introduced `mesos-build`, along with pre-built docker images.


Diffs
-

  support/jenkins/buildbot.sh 7f78509699b036df025c314fe913158f54402014 
  support/mesos-build.sh PRE-CREATION 
  support/mesos-build/centos-7.dockerfile PRE-CREATION 
  support/mesos-build/entrypoint.sh PRE-CREATION 
  support/mesos-build/ubuntu-16.04.dockerfile PRE-CREATION 


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


Testing
---


Thanks,

Michael Park

Review Request 65432: Added `examples/flags.hpp` to `src/Makefile.am`.

[Michael Park]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Added `examples/flags.hpp` to `src/Makefile.am`.


Diffs
-

  src/Makefile.am 155338673e1c23a2cfeab39bdf3ebe14568bfebc 


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


Testing
---


Thanks,

Michael Park

Review Request 65428: Reduced the output from Maven during builds.

[Michael Park]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/Makefile.am 155338673e1c23a2cfeab39bdf3ebe14568bfebc 


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


Testing
---


Thanks,

Michael Park

Review Request 65376: Reverted `protobuf::read(path)` to return `Result`.

[Michael Park]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

Due to the performance hits of `fsync`, the current implementation
of `state::checkpoint` does not invoke `fsync` prior to `rename`.

As a result, according to this blog post:

  https://thunk.org/tytso/blog/2009/03/12/delayed-allocation-and-
  the-zero-length-file-problem/

If the agent machine crashes after `rename` is called but before
the buffer has been flushed, the file will end up being empty.

This reverts commit 4f9cda17e1a747bc3c4ab3667569304e09600b29.


Diffs
-

  3rdparty/stout/include/stout/protobuf.hpp 
7e7659814a5d95c41c4dfabb5669fd061dff4716 


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


Testing
---


Thanks,

Michael Park

Review Request 65378: Reverted the uses of `state::read` since it returns `Result`.

[Michael Park]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

This reverts commit fda054b50ff7cdd2d7a60d31cfe24ce42bfbfaa5.


Diffs
-

  src/resource_provider/storage/provider.cpp 
1f30d0af4f75fc8aad3580669098bd42afabcf5c 
  src/slave/containerizer/mesos/paths.cpp 
186048f159d6ce20d9c5dc65c00012eb5911520b 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
168f786248ebdd6ff9855fe4998b27b169611f81 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
39eea34da78dd41d744dd82d564acdf4b3f6028a 
  src/slave/state.cpp 41c12f61520b2e769e3a1226bf0a767f779c2a75 
  src/tests/protobuf_io_tests.cpp 4cdf17fd2426159643c719a17fee6e6cedb40f55 
  src/tests/slave_recovery_tests.cpp 6dcbedb94a2c7b3d65770fc89920c0cad1298d1f 


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


Testing
---


Thanks,

Michael Park

Review Request 65377: Updated `state::read` to return `Result`.

[Michael Park]

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

Review request for mesos, Chun-Hung Hsiao and Jie Yu.


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


Repository: mesos


Description
---

Updated `state::read` to return `Result`.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
44cf9b211f82100b544c1d70e6ad5d6e8db7dec7 
  src/slave/containerizer/mesos/paths.cpp 
186048f159d6ce20d9c5dc65c00012eb5911520b 
  src/slave/state.hpp dcab4434a0d82efec33f55a1037c892d0a7e9d55 
  src/slave/state.cpp 41c12f61520b2e769e3a1226bf0a767f779c2a75 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 65375: Added move construction and assignment to Future.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/future.hpp
Lines 116-117 (patched)
<https://reviews.apache.org/r/65375/#comment276021>

We shouldn't need these, as the copy/move assignment operators will cover 
these scenarios already.
For example, we don't define these for `Option` and `Try`.


- Michael Park


On Jan. 28, 2018, 6:29 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65375/
> ---
> 
> (Updated Jan. 28, 2018, 6:29 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-2922
> https://issues.apache.org/jira/browse/MESOS-2922
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Includes using `default` where applicable.
> * Includes assignment operators from `const T&` and `T&&`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 2b8a0590e2e1708be5471fb5530194204b1fcdf1 
> 
> 
> Diff: https://reviews.apache.org/r/65375/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 65372: Replaced the `Os.Which` test to use `cat` rather than `ping`.

[Michael Park]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Starting from Ubuntu 15.04, `ping` is not installed by default.
Rather than inducing a silly dependency, we'll test with `cat` instead.


Diffs
-

  3rdparty/stout/tests/os_tests.cpp 11f17206688e09e86706d2d57d029f1c29d35e0d 


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


Testing
---

Successfully ran:
```bash
OS=ubuntu:16.04 COMPILER=gcc CONFIGURATION="--verbose 
--disable-libtool-wrappers" JOBS=32 BUILDTOOL=autotools ENVIRONMENT="GLOG_v=1 
MESOS_VERBOSE=1" support/docker-build.sh
```


Thanks,

Michael Park

Re: Review Request 65212: Updated `mesos-tidy` docker build setup for benchmarks.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 18, 2018, 5:24 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65212/
> ---
> 
> (Updated Jan. 18, 2018, 5:24 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the `mesos-tidy` docker build setup to generate
> `benchmarks.pb.h` needed by the libprocess benchmark files added as
> part of `eb901173b8aa43b9fd1ab0f727ff5187982c4449`.
> 
> We also reorder the targets to be build in the script so that all
> proto targets are located close to each other and after manually
> triggered 3rdparty dependencies (like e.g., protobuf). This should
> reduce redudant build invocations.
> 
> 
> Diffs
> -
> 
>   support/mesos-tidy/entrypoint.sh dd83d66f48968bce652bc78ccf2d807949f1725b 
> 
> 
> Diff: https://reviews.apache.org/r/65212/diff/2/
> 
> 
> Testing
> ---
> 
> Tested with a custom docker image and `support/mesos-tidy.sh` pointing to it.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61811: Flattened continuation chains in containerizer for readability.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 18, 2018, 5:11 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61811/
> ---
> 
> (Updated Jan. 18, 2018, 5:11 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f69f65a5033ce0c1fa2fe9591d8c6bbada5e09fb 
> 
> 
> Diff: https://reviews.apache.org/r/61811/diff/4/
> 
> 
> Testing
> ---
> 
> make check on:
> `Apple LLVM version 8.0.0 (clang-800.0.42.1)` on Mac OS 10.11.6
> `c++ (GCC) 6.2.1 20160916 (Red Hat 6.2.1-2)` on Fedora 24
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 65204: Eliminated some unnecessary copying in the agent's HTTP operator API.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 17, 2018, 5:19 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65204/
> ---
> 
> (Updated Jan. 17, 2018, 5:19 p.m.)
> 
> 
> Review request for mesos, Michael Park and Meng Zhu.
> 
> 
> Bugs: MESOS-8455
> https://issues.apache.org/jira/browse/MESOS-8455
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 94fb72c6e6fbc862314ecf2871ae025e422ac41e 
> 
> 
> Diff: https://reviews.apache.org/r/65204/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 65200: Added a default move constructor for Result.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 17, 2018, 3:30 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65200/
> ---
> 
> (Updated Jan. 17, 2018, 3:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-2921
> https://issues.apache.org/jira/browse/MESOS-2921
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/result.hpp 
> 0ce98fa208e4563036294448e2081b80884b9748 
> 
> 
> Diff: https://reviews.apache.org/r/65200/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 65149: MesosTidy: Enabled `mesos-this-capture`.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

MesosTidy: Enabled `mesos-this-capture`.


Diffs
-

  support/clang-tidy d68ddab6c796f1f583fb72ec06d6a91d845d1da9 


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


Testing
---

Ran `support/mesos-tidy.sh` and confirmed that the false positives have 
disappeared.


Thanks,

Michael Park

Re: Review Request 65063: Replace `stringify()` with `jsonify()` in http serialization.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Jan. 9, 2018, 11:06 p.m., Meng Zhu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65063/
> ---
> 
> (Updated Jan. 9, 2018, 11:06 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace `stringify()` with `jsonify()` in http serialization.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 3ada1f064b4bd001cd4d7dccc186641f475011a0 
> 
> 
> Diff: https://reviews.apache.org/r/65063/diff/1/
> 
> 
> Testing
> ---
> 
> Running on MacBook Pro with 3.3 GHz Intel Core i7, built with 
> `--enable-optimize`:
> 
> With `stringify()`:
> 
> Test setup: 2000 agents with a total of 10 running tasks and 10 
> completed tasks.
> Getstate application/json query response took 2.71121739318333mins
> 
> Test setup: 2000 agents with a total of 20 running tasks and 0 completed 
> tasks.
> Getstate application/json query response took 5.3197060575mins
> 
> Test setup: 2 agents with a total of 10 running tasks and 0 completed 
> tasks.
> Getstate application/json query response took 3.18027318681667mins
> 
> ---
> 
> With `jsonify()`:
> 
> Test setup: 2000 agents with a total of 10 running tasks and 10 
> completed tasks.
> Getstate application/json query response took 2.10034958736667mins
> 
> Test setup: 2000 agents with a total of 20 running tasks and 0 completed 
> tasks.
> Getstate application/json query response took 3.38100704925mins
> 
> Test setup: 2 agents with a total of 10 running tasks and 0 completed 
> tasks.
> Getstate application/json query response took 2.2140585963mins
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>

Re: Review Request 65022: Updated uses of `protobuf::read(path)` which now returns `Try`.

[Michael Park]

> On Jan. 9, 2018, 5:49 p.m., Benjamin Mahler wrote:
> > Looks like we need to take a closer look at the change being made w.r.t. to 
> > the none case now becoming an error.
> > 
> > Can you include vinod on this review?

Consulted Chun and Jie to audit this.

Since we introduced `state::checkpoint`, the atomic writes make it such that
handling of the `None` case became dead code. For example:

```cpp
  if (frameworkInfo.isNone()) {
// This could happen if the slave died after opening the file for
// writing but before it checkpointed anything.
LOG(WARNING) << "Found empty framework info file '" << path << "'";
return state;
  }
```

The unhandled `None` cases in new code (e.g., resource provider) is due to
the fact that we can't have an empty file.


In short, these changes are safe.


- Michael


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


On Jan. 8, 2018, 10:14 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65022/
> ---
> 
> (Updated Jan. 8, 2018, 10:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-8375
> https://issues.apache.org/jira/browse/MESOS-8375
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the path version of `protobuf::read` now returns `Try`,
> many of the existing code is removed and/or simplified.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 55f2e662ecfc72bc93a7101537e7a0dc16f11522 
>   src/slave/containerizer/mesos/paths.cpp 
> d6ea618b20431ac95f880045143d09366f1740bf 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
> 1ab66c1a6d2ebe6e7a3382529c85fddfa72f79dc 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 7621c4960047e099953a26602ef34682a0ba8a2a 
>   src/slave/state.cpp 5428b341b061c8209a5cfe63e17b71467d9f4c48 
>   src/tests/protobuf_io_tests.cpp 4a2e3a3bc87c1a3394368439e5dd30b410e468a4 
>   src/tests/slave_recovery_tests.cpp e305d7430b64409fdac84a33ee072707471d2c49 
> 
> 
> Diff: https://reviews.apache.org/r/65022/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 65022: Updated uses of `protobuf::read(path)` which now returns `Try`.

[Michael Park]

> On Jan. 10, 2018, 1:29 p.m., Chun-Hung Hsiao wrote:
> > src/slave/containerizer/mesos/paths.cpp
> > Line 263 (original), 263 (patched)
> > <https://reviews.apache.org/r/65022/diff/1/?file=1935567#file1935567line263>
> >
> > Return a `Try`.

Oops. Sorry, turns out this is actually correct because we have this code 
above..
```
  if (!os::exists(path)) {
// This is possible because we don't atomically create the
// directory and write the 'pid' file and thus we might
// terminate/restart after we've created the directory but
// before we've written the file.
return None();
  }
```


- Michael


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


On Jan. 8, 2018, 10:14 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65022/
> ---
> 
> (Updated Jan. 8, 2018, 10:14 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Chun-Hung Hsiao, and Vinod Kone.
> 
> 
> Bugs: MESOS-8375
> https://issues.apache.org/jira/browse/MESOS-8375
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the path version of `protobuf::read` now returns `Try`,
> many of the existing code is removed and/or simplified.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 55f2e662ecfc72bc93a7101537e7a0dc16f11522 
>   src/slave/containerizer/mesos/paths.cpp 
> d6ea618b20431ac95f880045143d09366f1740bf 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
> 1ab66c1a6d2ebe6e7a3382529c85fddfa72f79dc 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 7621c4960047e099953a26602ef34682a0ba8a2a 
>   src/slave/state.cpp 5428b341b061c8209a5cfe63e17b71467d9f4c48 
>   src/tests/protobuf_io_tests.cpp 4a2e3a3bc87c1a3394368439e5dd30b410e468a4 
>   src/tests/slave_recovery_tests.cpp e305d7430b64409fdac84a33ee072707471d2c49 
> 
> 
> Diff: https://reviews.apache.org/r/65022/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 65055: Fixed the order of parameters of `is_specialization_of` in libprocess.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Fixed the order of parameters of `is_specialization_of` in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/future.hpp 
54fbbeb9a3378298a71bb569b22ed5dd6d3c39ef 


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


Testing
---


Thanks,

Michael Park

Review Request 65054: Fixed the order of parameters of `is_specialization_of` in stout.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Given `std::is_base_of<T, U>`, the interpretation of the question is:
"is `T` base of `U`?". Similarly, in this case want to ask the question
"is some type a specialization of a template?" As such, the order of
template paramters should be `is_specialization_of<Type, Template>`.


Diffs
-

  3rdparty/stout/include/stout/traits.hpp 
5dea08404fd12f68af0538a16c5ee1d9734dd8c5 


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


Testing
---


Thanks,

Michael Park

Review Request 65031: Replaced `convertResourceFormat` with `downgradeResources` accordingly.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Replaced `convertResourceFormat` with `downgradeResources` accordingly.


Diffs
-

  src/master/registry_operations.hpp 41047f0b672ebbfe8d323221f37bf6b617e8b6ea 
  src/master/registry_operations.cpp 149009b07663f530c444d0b7205f23fe4510 


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


Testing
---


Thanks,

Michael Park

Review Request 65030: Replaced `convertResourceFormat` with `upgradeResources` appropriately.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Replaced `convertResourceFormat` with `upgradeResources` appropriately.


Diffs
-

  src/common/resources.cpp 1291208b9f2a7c373ff7ec941d3e4310fec92207 
  src/master/master.cpp 28d8be3a4769b418b61cff0b95845e4232135bc7 
  src/master/quota_handler.cpp b1757e7a1f64b0843c1213324c4d12986563a002 
  src/master/registry_operations.cpp 149009b07663f530c444d0b7205f23fe4510 
  src/slave/slave.cpp aeb0fdaeda78f26de2ec790735bfa88c5be72850 
  src/slave/validation.cpp 7444d6a5a78e8d7abd739b5ab564f3e1eca35c8b 
  src/tests/master_quota_tests.cpp 79c0739a60d6f1b5d104313856841751a924f0c0 
  src/tests/master_tests.cpp 5546fd937d078c1f757964d5163449ffd993388e 
  src/tests/persistent_volume_tests.cpp 
22ae2104b101cf70751b67774803aefce555b7e7 
  src/tests/reservation_tests.cpp c19d9442dd93c81be8dea1ea00b1cf723174068e 


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


Testing
---


Thanks,

Michael Park

Review Request 65029: Added `vector` overloads for `(down/up)gradeResources`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added `vector` overloads for `(down/up)gradeResources`.


Diffs
-

  src/common/resources_utils.hpp 611e267dc3da19f94fd1bfa060ce9ac71d12d328 
  src/common/resources_utils.cpp 967cfd7e3b021a486dd055d73dc0ad46f87ea1ef 


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


Testing
---


Thanks,

Michael Park

Review Request 65025: Replaced `os::read` with `state::read`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Replaced `os::read` with `state::read`.


Diffs
-

  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
deacffeef256bc05aae5325fdc19a11af5d508ba 
  src/slave/containerizer/mesos/paths.cpp 
d6ea618b20431ac95f880045143d09366f1740bf 
  src/slave/state.cpp 5428b341b061c8209a5cfe63e17b71467d9f4c48 
  src/tests/slave_recovery_tests.cpp e305d7430b64409fdac84a33ee072707471d2c49 


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


Testing
---


Thanks,

Michael Park

Review Request 65023: Added `state::read` to complement `state::checkpoint`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added `state::read` to complement `state::checkpoint`.


Diffs
-

  src/slave/state.hpp 01abb5095cf09dd36d7f154d007336892bd8df46 


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


Testing
---


Thanks,

Michael Park

Review Request 65026: Replaced `ResourcesState::recoverResources` with `state::read`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

The existing `ResourcesState::recoverResources` had complicated logic
for reading back the checkpointed resources. The logic is typically
used not merely for a sequence of messages, but rather for dynamically
appended sequence of messages (e.g. status updates). The `Resources`
however is not a dynamically appended sequence of messages, but rather
the full state and does not require truncation of a partial message.


Diffs
-

  src/slave/state.hpp 01abb5095cf09dd36d7f154d007336892bd8df46 
  src/slave/state.cpp 5428b341b061c8209a5cfe63e17b71467d9f4c48 
  src/tests/slave_recovery_tests.cpp e305d7430b64409fdac84a33ee072707471d2c49 


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


Testing
---


Thanks,

Michael Park

Review Request 65022: Updated uses of `protobuf::read(path)` which now returns `Try`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Since the path version of `protobuf::read` now returns `Try`,
many of the existing code is removed and/or simplified.


Diffs
-

  src/resource_provider/storage/provider.cpp 
55f2e662ecfc72bc93a7101537e7a0dc16f11522 
  src/slave/containerizer/mesos/paths.cpp 
d6ea618b20431ac95f880045143d09366f1740bf 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
1ab66c1a6d2ebe6e7a3382529c85fddfa72f79dc 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
7621c4960047e099953a26602ef34682a0ba8a2a 
  src/slave/state.cpp 5428b341b061c8209a5cfe63e17b71467d9f4c48 
  src/tests/protobuf_io_tests.cpp 4a2e3a3bc87c1a3394368439e5dd30b410e468a4 
  src/tests/slave_recovery_tests.cpp e305d7430b64409fdac84a33ee072707471d2c49 


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


Testing
---


Thanks,

Michael Park

Review Request 65024: Replaced `protobuf::read` with `state::read`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Replaced `protobuf::read` with `state::read`.


Diffs
-

  src/resource_provider/storage/provider.cpp 
55f2e662ecfc72bc93a7101537e7a0dc16f11522 
  src/slave/containerizer/mesos/paths.cpp 
d6ea618b20431ac95f880045143d09366f1740bf 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
1ab66c1a6d2ebe6e7a3382529c85fddfa72f79dc 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
7621c4960047e099953a26602ef34682a0ba8a2a 
  src/slave/state.cpp 5428b341b061c8209a5cfe63e17b71467d9f4c48 
  src/tests/slave_recovery_tests.cpp e305d7430b64409fdac84a33ee072707471d2c49 


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


Testing
---


Thanks,

Michael Park

Review Request 65021: Returned `Try` from `protobuf::read(path)` rather than `Result`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

The path version of `protobuf::read` used to return `Result` and
returned `None` only when the file is empty (`ignorePartial` is always
`false`). The `None` return represents EOF for the "streaming" version
of `protobuf::read` that takes an FD, but for the path version an empty
file when we expected to read `T` is simply an error. Thus, we map the
`None` return to an `Error` for the path version and return a `Try`.


Diffs
-

  3rdparty/stout/include/stout/protobuf.hpp 
f00a048fb0363f86cada97352a00a65a27f15bac 


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


Testing
---


Thanks,

Michael Park

Review Request 64924: Fixed the broken GRPC build.

[Michael Park]

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

Review request for mesos and Chun-Hung Hsiao.


Repository: mesos


Description
---

Added `upgradeResources` to the `resources_utils.hpp`, and a silly
mistake around using `foreach` over a protobuf mutable repeated field.


Diffs
-

  src/common/resources_utils.hpp 115efc2868be3deab7af3f9a30dd26ca3c49a8a6 
  src/resource_provider/storage/provider.cpp 
e2608723af0ef9955e46da300254306497d2d6df 


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


Testing
---


Thanks,

Michael Park

Review Request 64920: Introduced `upgradeResources` to complement `downgradeResources`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Introduced `upgradeResources` to complement `downgradeResources`.


Diffs
-

  src/common/resources_utils.hpp 115efc2868be3deab7af3f9a30dd26ca3c49a8a6 
  src/common/resources_utils.cpp bd9e025c29fc100121dd187603b2b116207762ba 


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


Testing
---


Thanks,

Michael Park

Review Request 64919: Updated the comment for `precomputeResourcesContainment`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

`precomputeResourcesContainment` used return a `bool`, but was modified
to return `void` since the `bool` is already included in the `result`.
This fixes the corresponding comment that was not adjusted accordingly.


Diffs
-

  src/common/resources_utils.cpp bd9e025c29fc100121dd187603b2b116207762ba 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Michael Park]

> On Jan. 2, 2018, 2:39 p.m., Benjamin Mahler wrote:
> > Is there a ticket for being more robust in the face of inability to recover 
> > a downgraded agent? Can you file one if not?
> > Also, do we want to target a ticket for 2.1 that stops downgrading? I 
> > assume the strategy to end the 1.x deprecation cycle will be that 2.1+ only 
> > supports being upgraded from 2.0.x.

Found the "downgrade capability" ticket: 
https://issues.apache.org/jira/browse/MESOS-7682


- Michael


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


On Jan. 2, 2018, 3:39 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64739/
> ---
> 
> (Updated Jan. 2, 2018, 3:39 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Upgraded resources that come from `protobuf::read`.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> ee9ac901a6f46b6c6749381d859b3b090c113673 
>   src/slave/containerizer/mesos/paths.cpp 
> 8a188a918873eef468a984b80f5ea7ebaa8fb923 
> 
> 
> Diff: https://reviews.apache.org/r/64739/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Michael Park]

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

(Updated Jan. 2, 2018, 3:39 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


Repository: mesos


Description (updated)
---

Upgraded resources that come from `protobuf::read`.


Diffs (updated)
-

  src/resource_provider/storage/provider.cpp 
ee9ac901a6f46b6c6749381d859b3b090c113673 
  src/slave/containerizer/mesos/paths.cpp 
8a188a918873eef468a984b80f5ea7ebaa8fb923 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.

[Michael Park]

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

(Updated Jan. 2, 2018, 3:34 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Set required fields in `TaskInfo`.


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


Repository: mesos


Description
---

Previously, our `downgradeResources` function only took a pointer to
`RepeatedPtrField`. This wasn't great since we needed manually
invoke `downgradeResources` on every `Resource`s field of every message.

This patch makes it such that `downgradeResources` can take any
`protobuf::Message` or `protobuf::RepeatedPtrField`.


Diffs (updated)
-

  src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
  src/common/resources_utils.cpp 47ba885517bdfef4764deb0826e13538ce529902 
  src/tests/resources_tests.cpp 64bde85e0e7f0bd395189eb6a8635383095b2f07 


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

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


Testing
---

Added new tests + `make check`


Thanks,

Michael Park

Re: Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Michael Park]

> On Dec. 21, 2017, 6:55 p.m., Benjamin Mahler wrote:
> > Hm.. I was a little puzzled here, why do some use convert with POST and 
> > some use the reflection based upgrade? Shouldn't we just use the reflection 
> > based upgrade everywhere?

Sorry for the confusion. The `upgradeResources` used here is the one pulled out 
of
`validateAndUpgradeResources`, and is not "reflection based upgrade". I called
it `upgradeResources` for now in the hopes that when we do introduce 
reflection-based
`upgradeResources`, the callsite wouldn't need to change. Although looking at 
it now
we'll probably be invoking the reflection-based upgrade on all of 
`resourceProviderState`
i.e., `upgradeResources(resourceProviderState.get());`


- Michael


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


On Dec. 19, 2017, 4:51 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64739/
> ---
> 
> (Updated Dec. 19, 2017, 4:51 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/storage/provider.cpp 
> 79d7f602bbc57bce308157dfbb6c2afaef806f23 
>   src/slave/containerizer/mesos/paths.cpp 
> 8a188a918873eef468a984b80f5ea7ebaa8fb923 
> 
> 
> Diff: https://reviews.apache.org/r/64739/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.

[Michael Park]

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

(Updated Jan. 2, 2018, 12:55 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Addressed BenM's comments.


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


Repository: mesos


Description
---

Previously, our `downgradeResources` function only took a pointer to
`RepeatedPtrField`. This wasn't great since we needed manually
invoke `downgradeResources` on every `Resource`s field of every message.

This patch makes it such that `downgradeResources` can take any
`protobuf::Message` or `protobuf::RepeatedPtrField`.


Diffs (updated)
-

  src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
  src/common/resources_utils.cpp 47ba885517bdfef4764deb0826e13538ce529902 
  src/tests/resources_tests.cpp 64bde85e0e7f0bd395189eb6a8635383095b2f07 


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

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


Testing
---

Added new tests + `make check`


Thanks,

Michael Park

Review Request 64895: Renamed "normalize" to "upgrade" in resources adjustment operations.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

We have introduced the notion of "downgrading" resources in order to
support downgrades of various components. The opposite operation used
to be called "normalize", but it seems "upgrade" would be simpler.


Diffs
-

  src/common/resources_utils.hpp 5b74ff2dd3ecb1a0101671d11ea10e29a43524b0 
  src/common/resources_utils.cpp 47ba885517bdfef4764deb0826e13538ce529902 
  src/master/http.cpp d7276e45e11f680e909027566b3cf29af10882c9 
  src/master/master.cpp 03eb178fa1af7d55ae387e6cb42cdc8d721a2196 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 63976: Modified `downgradeResources` to use protobuf reflection.

[Michael Park]

> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Line 175 (original), 175-176 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924144#file1924144line175>
> >
> > s/does/do/?
> > 
> > Maybe remove the first sentence and just directly say "all-or-nothing"? 
> > Atomicity seems a little obscure here to me

Removed the first sentence.


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Lines 181-182 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924144#file1924144line181>
> >
> > Does it keep going and finish the rest in the error case? Or does it 
> > stop converting and return the error? From reading the above comments, it 
> > sounds like it wouldn't break early.

The current behavior is that it stops converting and returns the error. I'll 
update the comment accordingly.


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.hpp
> > Lines 184-185 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924144#file1924144line184>
> >
> > Are you saying that once a component has refined reservations, it 
> > cannot be downgraded back to.. the version before we had reservation 
> > refinement? As written, it seems to be speaking too generally about 
> > downgrades: whether 1.9 could be downgraded to 1.8 is a separate question?

Yeah. Good point. Updated to include "... to a version that does not support 
reservation refinement."


> On Dec. 20, 2017, 6:24 p.m., Benjamin Mahler wrote:
> > src/common/resources_utils.cpp
> > Lines 779-781 (patched)
> > <https://reviews.apache.org/r/63976/diff/2/?file=1924145#file1924145line783>
> >
> > Hm.. is returning bool here an optimization? I would imagine the 
> > resultant map could (or I guess already does) contain the top level 
> > descriptor?
> > 
> > Then, downgradeResourcesImpl would first check the passed in message 
> > descriptor and bail if not contained?
> > 
> > ```
> > static Try downgradeResourcesImpl(
> > Message* message,
> > const hashmap& resourcesContainment)
> > {
> >   CHECK_NOTNULL(message);
> > 
> >   const Descriptor* descriptor = message->GetDescriptor();
> >   
> >   if (!resourcesContainment.at(descriptor)) {
> > return; // Done.
> >   }
> >   
> >   if (descriptor == mesos::Resource::descriptor()) {
> > return downgradeResources(static_cast<mesos::Resource*>(message));
> >   }
> >   
> >   // When recursing into fields, I guess we don't bother checking
> >   // containement before recursing since the recursive call will check?
> > ```
> > 
> > Then the top level function gets a bit simpler?
> > 
> > ```
> > Try downgradeResources(Message* message)
> > {
> >   const Descriptor* descriptor = message->GetDescriptor();
> > 
> >   hashmap resourcesContainment;
> >   internal::precomputeResourcesContainment(descriptor, 
> > );
> >   
> >   return internal::downgradeResourcesImpl(message, 
> > resourcesContainment);
> > ```
> > 
> > Just curious to get your thoughts on the two approaches.

I've removed the `bool` return since it was kind of a silly, and was not even 
an optimization.

However, I believe moving the containment check inside `downgradeResourcesImpl` 
would be
a regression in performance. Consider a protobuf message:
```
message A {
  repeated B messages;
  optional Resource resource;
}
```
and suppose `B` is not `Resource` nor does it have any `Resource`s within.

Since `A` has `resource`, we'll pass the `resourcesContainment.at(descriptor)` 
check.

Ideally we would skip `messages` entirely, since from the schema of `B` we know 
that
there aren't any `Resource`s in there. If we were to wait until the recursive 
call,
we would fully execute this loop going over each `B` in `messages`:
```
  const int size = reflection->FieldSize(*message, field);

  for (int j = 0; j < size; ++j) {
Try result = downgradeResourcesImpl(
reflection->MutableRepeatedMessage(message, field, j),
resourcesContainment);

if (result.isError()) {
  return result;
}
  }
```

I think the issue is mainly in the way in which we need to treat repeated 
fields.
If we were able to get access to a repeated field of `Message`s and

Re: Review Request 64738: Pulled out `upgradeResources` out of `validateAndNormalizeResources`.

[Michael Park]

> On Dec. 19, 2017, 6:13 p.m., Benjamin Mahler wrote:
> > The "upgrade" vs "normalize" inconsistency seems a little odd?

I'll follow up with a patch to change "normalize" to "upgrade".
Seems more fitting with the presence of "downgrade".


- Michael


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


On Dec. 19, 2017, 4:51 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64738/
> ---
> 
> (Updated Dec. 19, 2017, 4:51 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/common/resources_utils.cpp 1676b72a9ad15bf8b131698a0600a1b0937c00b4 
> 
> 
> Diff: https://reviews.apache.org/r/64738/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 64686: Fixed the type-punned pointer and strict aliasing issue.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 22, 2017, 5:42 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64686/
> ---
> 
> (Updated Dec. 22, 2017, 5:42 a.m.)
> 
> 
> Review request for mesos, Armand Grillet, Benjamin Bannier, and Michael Park.
> 
> 
> Bugs: MESOS-6616
> https://issues.apache.org/jira/browse/MESOS-6616
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Dereferencing a pointer cast from a different type of pointer
> violates the so-called “strict aliasing” rule, which is undefined
> behaviour and might lead to bugs when compiler optimizations are
> enabled.
> 
> For more information on this topic, see
> https://blog.regehr.org/archives/959
> http://alas.matf.bg.ac.rs/manuals/lspe/snode=153.html
> 
> 
> Diffs
> -
> 
>   src/linux/ns.cpp 5e2df1ed56432c6c1bfa04a31dba7f9f547d6139 
> 
> 
> Diff: https://reviews.apache.org/r/64686/diff/3/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6 and various Linux distros
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 63978: Added tests for protobuf reflection-based `downgradeResources`.

[Michael Park]

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

Review request for mesos.


Repository: mesos


Description
---

Added tests for protobuf reflection-based `downgradeResources`.


Testing
---


Thanks,

Michael Park

Review Request 64739: Upgraded resources that come from `protobuf::read`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/resource_provider/storage/provider.cpp 
79d7f602bbc57bce308157dfbb6c2afaef806f23 
  src/slave/containerizer/mesos/paths.cpp 
8a188a918873eef468a984b80f5ea7ebaa8fb923 


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


Testing
---


Thanks,

Michael Park

Review Request 64738: Pulled out `upgradeResources` out of `validateAndNormalizeResources`.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/common/resources_utils.cpp 1676b72a9ad15bf8b131698a0600a1b0937c00b4 


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


Testing
---


Thanks,

Michael Park

Review Request 64737: Added validation / normalized resources for storage-related operations.

[Michael Park]

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

Review request for mesos.


Repository: mesos


Description
---

Added validation / normalized resources for storage-related operations.


Diffs
-

  src/common/resources_utils.cpp 1676b72a9ad15bf8b131698a0600a1b0937c00b4 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 63951: Added noexcept specifier to Option.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/option.hpp
Lines 101 (patched)
<https://reviews.apache.org/r/63951/#comment271801>

This condition is implied from the constructibility requirement. The 
standard doesn't specify these, for example.


- Michael Park


On Nov. 20, 2017, 6:38 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63951/
> ---
> 
> (Updated Nov. 20, 2017, 6:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `noexcept` on move constructor and move assignment operator allows STL
> to use some optimizations, e.g. `std::vector` can move elements, when
> growing, rather than making copies.
> Specifically, `mesos::Resources::Resource_` generated constructor is
> deduced as `noexcept` after this change, improving performance of
> operations on `mesos::Resources`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/option.hpp 
> 32133f95654b9367bce5c354142b0ae91146ec3d 
> 
> 
> Diff: https://reviews.apache.org/r/63951/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> Verified with
> ```
> static_assert(std::is_nothrow_move_constructible::value,
>  "");
> ```
> before and after changes.
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64428: Eliminated some copying of tasks / executors in agent re-registration.

[Michael Park]

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


Fix it, then Ship it!





src/master/master.cpp
Lines 6675-6684 (original)
<https://reviews.apache.org/r/64428/#comment271721>

It looks like we still do all of these (via move) except `tasks`. I 
would've expected to see these still here but turned into moves. What was the 
motivation for the new pattern?



src/master/master.cpp
Lines 6690 (patched)
<https://reviews.apache.org/r/64428/#comment271720>

Shouldn't need `std::cref` here.


- Michael Park


On Dec. 7, 2017, 12:46 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64428/
> ---
> 
> (Updated Dec. 7, 2017, 12:46 p.m.)
> 
> 
> Review request for mesos, Dmitry Zhuk and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 2658312b0d10a72fefda68c7d3137b94afbd8249 
>   src/master/master.cpp 2fd66c072e9a194680d7653c664bd8a68ea1d2f0 
> 
> 
> Diff: https://reviews.apache.org/r/64428/diff/3/
> 
> 
> Testing
> ---
> 
> About a 10-20% improvement.
> 
> Before:
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 2.977767303secs
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 5.642229947secs
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 6.966318315secs
> 
> After:
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 2.591612374secs
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 4.52633125secs
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 6.449984263secs
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 64427: Added a RepeatedPtrField to vector conversion overload for rvalues.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 7, 2017, 12:55 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64427/
> ---
> 
> (Updated Dec. 7, 2017, 12:55 p.m.)
> 
> 
> Review request for mesos, Dmitry Zhuk and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This enables moving the individual entries out into the output vector.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/protobuf.hpp 
> 257832ed47eaa087537a46a974153ecd65c5892c 
> 
> 
> Diff: https://reviews.apache.org/r/64427/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Review Request 64391: Fixed incorrect testing of existence of variables.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

A change was made in a response to a Javascript linter, but
the corresponding change was incorrect:
https://github.com/apache/mesos/commit/af8404e7bf49201d7f3a0a563f43d767539558f0#diff-c4d2d75ab288353e2104377479e53a00L88

Specifically:
```
 -page_size || $.error('Expecting page_size to be defined');
 -truncate_length || $.error('Expecting truncate_length to be defined');
 +if (page_size) {
 +$.error('Expecting page_size to be defined')
 +}
 +
 +if (truncate_length) {
 +$.error('Expecting truncate_length to be defined')
 +}
```


Diffs
-

  src/webui/master/static/js/jquery.pailer.js 
93ff04bac328d72a45317b2f15ccbc5e45c161d4 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 63915: Reduced tasks copying during agent reregistration.

[Michael Park]

> On Dec. 4, 2017, 11:37 a.m., Michael Park wrote:
> > src/master/master.hpp
> > Line 128 (original), 128 (patched)
> > <https://reviews.apache.org/r/63915/diff/3/?file=1904570#file1904570line128>
> >
> > Maybe we can consider making this `vector&&` as discussed in 
> > https://reviews.apache.org/r/63914/
> 
> Dmitry Zhuk wrote:
> This will make some difficulties for `registerSlave` which also uses this 
> constructor. Shall we migrate `registerSlave` to using message similar to 
> `reregisterSlave`?

That sounds ideal, but I don't think it should necessarily be part of this 
work. We'll follow-up on it.


- Michael


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


On Nov. 21, 2017, 9:53 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63915/
> ---
> 
> (Updated Nov. 21, 2017, 9:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tasks can be moved into master's internal data structures from message
> to save some cycles on copying the data.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0f8a2ac72c3484f911853c2994fc71a488d66d96 
>   src/master/master.cpp fadc78b2ca5d46b8cc12a794b428753aa79ac095 
> 
> 
> Diff: https://reviews.apache.org/r/63915/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran benchmark with `--enable-optimize --enable-lock-free-run-queue 
> --enable-lock-free-event-queue 
> --enable-last-in-first-out-fixed-size-semaphore`
> `./mesos-tests.sh --benchmark 
> --gtest_filter=AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.*`
> On 2608c0b8f62a9359d3d23e1724b6e91f316cfc76 (includes protobuf-3.5.0):
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 16.202206916secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (30065 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 30.509804836secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (57145 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 22.581999748secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (39629 ms)
> ```
> 
> On this chain of patches:
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 8.456615936secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (22659 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 15.09102354secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (43828 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 16.122729767secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (33182 ms)
> ```
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63761: Replaced std::shared_ptr with std::unique_ptr in dispatch.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 10:07 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63761/
> ---
> 
> (Updated Dec. 5, 2017, 10:07 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since `dispatch` can now handle movable only parameters, `Promise` and
> functor can be safely wrapped into `std::unique_ptr` for efficiency.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 155f362a888caef5ce2db803a109400ca4c7ec37 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
> 
> 
> Diff: https://reviews.apache.org/r/63761/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63913: Replaced std::shared_ptr with std::unique_ptr in Future.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 21, 2017, 9:52 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63913/
> ---
> 
> (Updated Nov. 21, 2017, 9:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced std::shared_ptr with std::unique_ptr in Future.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> d48dfd795fdc4021270621522a365e3cad4102a6 
> 
> 
> Diff: https://reviews.apache.org/r/63913/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63641: Used move for events consumption in master.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 7, 2017, 9:42 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63641/
> ---
> 
> (Updated Nov. 7, 2017, 9:42 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used move for events consumption in master.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 448be3b4a3eb3d0cbd34a39b19640d15cf3fc2af 
>   src/master/master.cpp 31991b5f7f7f4e7dc73d71a3160348bfc0a627c3 
> 
> 
> Diff: https://reviews.apache.org/r/63641/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63638: Added callable once support in Future.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 21, 2017, 9:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63638/
> ---
> 
> (Updated Nov. 21, 2017, 9:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Future` guarantees that callbacks are called at most once, so it can
> use `lambda::CallableOnce` to expicitly declare this, and allow
> corresponding optimizations with moves.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> d48dfd795fdc4021270621522a365e3cad4102a6 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> bb7a1c83e136136088e60e4d187e2a52df49f454 
> 
> 
> Diff: https://reviews.apache.org/r/63638/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63637: Added callable once support in defer.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 4:20 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63637/
> ---
> 
> (Updated Dec. 5, 2017, 4:20 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows `defer` result to be converted to `CallableOnce`, ensuring
> that bound arguments are moved, when call is made, and avoiding making
> copies of bound arguments.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/deferred.hpp 
> 950f1cccaf93298664a95ba1fd2c5d2a42deb725 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> d1b30b0754c55fd765885ddcfe9f9f68ba509ac1 
> 
> 
> Diff: https://reviews.apache.org/r/63637/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64338: Reduced number of supported arguments in _Deferred conversion operators.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 4:18 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64338/
> ---
> 
> (Updated Dec. 5, 2017, 4:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Conversion of `_Deferred` to `std::function` and `Deferred` currently
> supports up to 12 parameters in function signature. However, this is
> unnecessary and is a huge overhead. Most usages require just one
> parameter (e.g. when `defer` is used with `Future`). And there are few
> usages with two parameters (in `master.cpp` to initialize allocator, and
> in `slave.cpp` to install signal handler).
> This number of parameters is different from the number of parameters
> passed to `defer`, but it's related and can be defined as maximum number
> of placeholders that can be passed to `defer`.
> 
> Given that `deferred.hpp` is indirectly included in most source files,
> it is beneficial to keep this number low. This patch changes maximum
> number of parameters to 2.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/deferred.hpp 
> 950f1cccaf93298664a95ba1fd2c5d2a42deb725 
> 
> 
> Diff: https://reviews.apache.org/r/64338/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63635: Prepared defer for use in callable once contexts.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63635/
> ---
> 
> (Updated Nov. 7, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes `defer` to use `lambda::partial` instead of `std::bind`,
> which allows it be used in callbale once contexts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/defer.hpp 
> 1c114a8d8e3a90fed9be0519ac093e402ebcd977 
>   3rdparty/libprocess/include/process/deferred.hpp 
> 950f1cccaf93298664a95ba1fd2c5d2a42deb725 
> 
> 
> Diff: https://reviews.apache.org/r/63635/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64347: Made libprocess event movable only.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 10:06 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64347/
> ---
> 
> (Updated Dec. 5, 2017, 10:06 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made libprocess events movable only.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
> 
> 
> Diff: https://reviews.apache.org/r/64347/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63631: Separated event visiting and consumption.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/event.hpp
Lines 91-127 (original), 103-144 (patched)
<https://reviews.apache.org/r/63631/#comment271214>

I noticed that we have 2 types of messages:
  (1) copyable but not assignable
  (2) not copyable nor assignable

I think we should make all of the message properly move-only. (e.g., the 
changes we made to `HttpEvent`).
For most of them we just need to remove the `const` on the members.



3rdparty/libprocess/include/process/event.hpp
Lines 114-117 (original), 126-129 (patched)
<https://reviews.apache.org/r/63631/#comment271209>

This comment needs to be updated.



3rdparty/libprocess/include/process/event.hpp
Lines 155-158 (original), 178-181 (patched)
<https://reviews.apache.org/r/63631/#comment271210>

Let's clean these up and pull them up and declare them `= delete;`.
Here and below.


- Michael Park


On Dec. 4, 2017, 8:01 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63631/
> ---
> 
> (Updated Dec. 4, 2017, 8:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This introduces `EventConsumer` interface, which add support of events
> with movable only data. This allows consumers to move data out of
> event, rather than copying it. This is required to implement movable
> only objects support in `defer` to guarantee that deferred functor is
> invoked only once, allowing deferred parameters to be moved into call.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
>   3rdparty/libprocess/include/process/process.hpp 
> d88c2f241a04cac3ec2e1d9fd4b323643ad24ae4 
>   3rdparty/libprocess/include/process/protobuf.hpp 
> 08605db4aa7ae782f1e5a1f692db9d46f899f842 
>   3rdparty/libprocess/include/process/run.hpp 
> 46a182b2dd4ab7add6d6f547e83958b19fa2a0f2 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 2c6de626c3fdf5ebd3a7aa5793553215c42dd494 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> d1b30b0754c55fd765885ddcfe9f9f68ba509ac1 
> 
> 
> Diff: https://reviews.apache.org/r/63631/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64337: Fixed CallableOnce operator() signature.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 5, 2017, 4:13 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64337/
> ---
> 
> (Updated Dec. 5, 2017, 4:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This changes `operator()` signature to match the one defined in
> `CallableOnce` template parameter.
> Previously used form incorrectly specifies that `operator()` can be
> invoked with arbitrary number and types of parameters, which can break
> other templates using SFINAE to check if functor can be invoked with
> specific parameters.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/lambda.hpp 
> 96586bd572093d2874a617252ed22b9e6874236b 
> 
> 
> Diff: https://reviews.apache.org/r/64337/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Review Request 64332: Removed `constexpr`-ness from `cpp17::invoke`.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

`std::invoke` is not marked `constexpr`.


Diffs
-

  3rdparty/stout/include/stout/cpp17.hpp 
3c4cc5345c94270785fa1594d7310af2b4dcf402 


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


Testing
---


Thanks,

Michael Park

Review Request 64331: Added more `cpp17::invoke` test cases.

[Michael Park]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/stout/tests/cpp17_tests.cpp 37d817f1af90cc6a17c5d7f17cf0571fd3f4ac7c 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 64312: Added reservation refinement documentation.

[Michael Park]

> On Dec. 4, 2017, 2:08 p.m., Benjamin Mahler wrote:
> > docs/upgrades.md
> > Lines 108 (patched)
> > <https://reviews.apache.org/r/64312/diff/2/?file=1908346#file1908346line108>
> >
> > Hm.. I'm not sure we should mention this here, it's a bit confusing to 
> > metion an "internal Mesos c++ library" as part of the framework API? We 
> > could have a c++ public headers section but since we break it all the time 
> > and haven't been publishing the changes here I'd probably shy away from 
> > that.

Yeah, this was a specific request from Joerg because he was working with some 
people who are using our internal libraries and these changes broke them :(


- Michael


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


On Dec. 4, 2017, 1:58 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64312/
> ---
> 
> (Updated Dec. 4, 2017, 1:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Joerg Schad.
> 
> 
> Bugs: MESOS-7663
> https://issues.apache.org/jira/browse/MESOS-7663
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added reservation refinement documentation.
> 
> 
> Diffs
> -
> 
>   docs/reservation.md 3a42783deb5b8ad288e626612c196bfd8439d299 
>   docs/upgrades.md 33ba4a69e6e53a313e89a3a24448e3e551443e23 
> 
> 
> Diff: https://reviews.apache.org/r/64312/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 64312: Added reservation refinement documentation.

[Michael Park]

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

(Updated Dec. 4, 2017, 1:58 p.m.)


Review request for mesos, Benjamin Mahler and Joerg Schad.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Added reservation refinement documentation.


Diffs (updated)
-

  docs/reservation.md 3a42783deb5b8ad288e626612c196bfd8439d299 
  docs/upgrades.md 33ba4a69e6e53a313e89a3a24448e3e551443e23 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 63915: Reduced tasks copying during agent reregistration.

[Michael Park]

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


Fix it, then Ship it!





src/master/master.hpp
Line 128 (original), 128 (patched)
<https://reviews.apache.org/r/63915/#comment271003>

Maybe we can consider making this `vector&&` as discussed in 
https://reviews.apache.org/r/63914/


- Michael Park


On Nov. 21, 2017, 9:53 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63915/
> ---
> 
> (Updated Nov. 21, 2017, 9:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Tasks can be moved into master's internal data structures from message
> to save some cycles on copying the data.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0f8a2ac72c3484f911853c2994fc71a488d66d96 
>   src/master/master.cpp fadc78b2ca5d46b8cc12a794b428753aa79ac095 
> 
> 
> Diff: https://reviews.apache.org/r/63915/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Ran benchmark with `--enable-optimize --enable-lock-free-run-queue 
> --enable-lock-free-event-queue 
> --enable-last-in-first-out-fixed-size-semaphore`
> `./mesos-tests.sh --benchmark 
> --gtest_filter=AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.*`
> On 2608c0b8f62a9359d3d23e1724b6e91f316cfc76 (includes protobuf-3.5.0):
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 16.202206916secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (30065 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 30.509804836secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (57145 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 22.581999748secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (39629 ms)
> ```
> 
> On this chain of patches:
> ```
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 10 running tasks and 10 
> completed tasks in 8.456615936secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/0
>  (22659 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
> Starting reregistration for all agents
> Reregistered 2000 agents with a total of 20 running tasks and 0 completed 
> tasks in 15.09102354secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/1
>  (43828 ms)
> [ RUN  ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
> Starting reregistration for all agents
> Reregistered 2 agents with a total of 10 running tasks and 0 
> completed tasks in 16.122729767secs
> [   OK ] 
> AgentFrameworkTaskCount/MasterFailover_BENCHMARK_Test.AgentReregistrationDelay/2
>  (33182 ms)
> ```
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63634: Changed dispatch to use callable once functors.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63634/
> ---
> 
> (Updated Nov. 7, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `dispatch` guarantees that functor will be called at most once, and
> therefore it allows optimizations, such as moves of deferred objects.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 155f362a888caef5ce2db803a109400ca4c7ec37 
>   3rdparty/libprocess/include/process/event.hpp 
> 571ffb1ea5c4787f10c2f4dee249aadc8b904bc6 
>   3rdparty/libprocess/src/process.cpp 
> 64bcce215d19558dd493e30e96ca16577fe0722a 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> d1b30b0754c55fd765885ddcfe9f9f68ba509ac1 
> 
> 
> Diff: https://reviews.apache.org/r/63634/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Review Request 64312: Added reservation refinement documentation.

[Michael Park]

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

Review request for mesos, Benjamin Mahler and Joerg Schad.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/reservation.md 3a42783deb5b8ad288e626612c196bfd8439d299 
  docs/task-state-reasons.md 8cf0c53d07a4db49437f52c7d7f991f621d523ac 
  docs/upgrades.md 33ba4a69e6e53a313e89a3a24448e3e551443e23 
  include/mesos/resource_provider/resource_provider.hpp 
45cda4251af6c437841609fbf611108b2955422e 
  include/mesos/resource_provider/resource_provider.proto 
2ce71f4c3e97e89c226966c347c2817e2bab9c1b 
  include/mesos/type_utils.hpp b786d0e1b887306e68561ce7c9907fa844d5786a 
  include/mesos/v1/mesos.hpp f393ed5654ad6d0b3eb5e28548c972779b082425 
  include/mesos/v1/resource_provider/resource_provider.hpp 
3fc45c94df2678b2c3a778eb3b7acbbd0d0446b7 
  include/mesos/v1/resource_provider/resource_provider.proto 
465b11dafd7c2e3b9476696ed75c1c077d6c8eeb 
  src/common/type_utils.cpp 1b466fcfd133b6fd0067668c7ba54d1ce80c8702 
  src/resource_provider/manager.hpp c2aeb15b0b8ebd167ddf9c42c9fc396d4c0a126b 
  src/resource_provider/manager.cpp 8d8b2f1396230d6edba590b8b7e9b4ca51366efe 
  src/resource_provider/validation.cpp 984d5934953c1f0a16a400fb2a2cc2cc5eb2ed89 
  src/tests/resource_provider_manager_tests.cpp 
a4c19ca769e66110d9aba0bae4792df9db3fed01 
  src/v1/mesos.cpp 2c81b37468dc27da863eb8a56a213436b94b73de 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 63914: Changed agent reregistration to work with message directly.

[Michael Park]

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


Fix it, then Ship it!





src/master/master.cpp
Line 6433 (original), 6394 (patched)
<https://reviews.apache.org/r/63914/#comment270977>

I think these `message` to `message_` changes should either take the type 
of the mesage (e.g., `shutdown`), or simply a `s/message/outgoing/`. Having 
unrelated `message` and `message_` in the same scope will likely cause 
confusion.



src/master/master.cpp
Line 6823 (original), 6779 (patched)
<https://reviews.apache.org/r/63914/#comment270974>

I think the intention is that we move this, right? I think the `Slave` ctor 
was updated on the next patch to take `vector`, rather than `vector&&`. Maybe 
we should make it `vector&&` such that we don't run into this?

Same with `agentCapabilities` and other related data


- Michael Park


On Nov. 21, 2017, 9:53 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63914/
> ---
> 
> (Updated Nov. 21, 2017, 9:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `reregisterSlave` now accepts `ReregisterSlaveMessage&&`, which opts-out
> of using protobuf arena, and allows passing message through dispatch
> chain without making any copies.
> Conversion of repeated message fields to `std::vector`s is performed
> only when needed.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 0f8a2ac72c3484f911853c2994fc71a488d66d96 
>   src/master/master.cpp fadc78b2ca5d46b8cc12a794b428753aa79ac095 
>   src/master/validation.hpp ac54062ea09f97ad96bd17deb106ea89a57f394a 
>   src/master/validation.cpp 8b5848bfd8c069f34a92a9a68597955c6e0c2ee2 
>   src/tests/master_validation_tests.cpp 
> 0e1c8b490ebe10bc50b8b6b530fa85128b967584 
> 
> 
> Diff: https://reviews.apache.org/r/63914/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63630: Added support for callable once functors.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/lambda.hpp
Lines 330 (patched)
<https://reviews.apache.org/r/63630/#comment270965>

`A/Args/`



3rdparty/stout/include/stout/lambda.hpp
Lines 345-349 (patched)
<https://reviews.apache.org/r/63630/#comment270963>

These fit in one line.



3rdparty/stout/include/stout/lambda.hpp
Lines 351-354 (patched)
<https://reviews.apache.org/r/63630/#comment270964>

Can't we just take `A&&... args` as the param types and forward it through 
like `std::forward(args)...`?



3rdparty/stout/include/stout/lambda.hpp
Lines 360 (patched)
<https://reviews.apache.org/r/63630/#comment270962>

Is this required for something? I'd prefer to get rid of it if we're not 
using it. If we do need it for something, let's make it a `using` declaration.



3rdparty/stout/include/stout/lambda.hpp
Lines 384 (patched)
<https://reviews.apache.org/r/63630/#comment270961>

`f == nullptr`


- Michael Park


On Dec. 4, 2017, 8 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63630/
> ---
> 
> (Updated Dec. 4, 2017, 8 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `CallableOnce` class is similar to `std::function`, but allows it to be
> called only once. Together with `partial` this provides foundation for
> copy-less `defer`, `dispatch` and `Future`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/lambda.hpp 
> a61d97b69e7eebd057c94148d39c6e1fc3066017 
> 
> 
> Diff: https://reviews.apache.org/r/63630/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64274: Added `lambda::partial` to .

[Michael Park]

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

(Updated Dec. 4, 2017, 7:21 a.m.)


Review request for mesos and Dmitry Zhuk.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added `lambda::partial` to .


Diffs (updated)
-

  3rdparty/stout/include/stout/lambda.hpp 
a61d97b69e7eebd057c94148d39c6e1fc3066017 
  3rdparty/stout/tests/lambda_tests.cpp 
ad8c2efddb6b64184670d0cfb33188ef843351ab 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 63631: Separated event visiting and consumption.

[Michael Park]

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




3rdparty/libprocess/include/process/event.hpp
Lines 172 (patched)
<https://reviews.apache.org/r/63631/#comment270934>

It looks like `consume` in this case happens to not actually move the 
`HttpEvent`, but if it were to we'd be in trouble due to the fact that 
`HttpEvent` holds a raw pointer and the move ctor is not defined accordingly.


- Michael Park


On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63631/
> ---
> 
> (Updated Nov. 7, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This introduces `EventConsumer` interface, which add support of events
> with movable only data. This allows consumers to move data out of
> event, rather than copying it. This is required to implement movable
> only objects support in `defer` to guarantee that deferred functor is
> invoked only once, allowing deferred parameters to be moved into call.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 2e246205020c3c5b8c2eb5187a8eb3643d1e6d4d 
>   3rdparty/libprocess/include/process/process.hpp 
> dc3375ce62556322eb2bc60ade61f313ade123b8 
>   3rdparty/libprocess/include/process/protobuf.hpp 
> fe152f273332470ac50f9715291897bb04cf95b9 
>   3rdparty/libprocess/include/process/run.hpp 
> 1540a78e52a90d4c1d4165c46be353caaad21bce 
>   3rdparty/libprocess/src/process.cpp 
> 71ae7129ffbd0e22eda2863b17bbcf588298c37b 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> e6c77d565d5acf72b475a085e9504679253b4b97 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 952c92c033e2363cff0c2c68610d3820b97d177e 
> 
> 
> Diff: https://reviews.apache.org/r/63631/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Review Request 64274: Added `lambda::partial` to .

[Michael Park]

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

Review request for mesos and Dmitry Zhuk.


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


Repository: mesos


Description
---

Added `lambda::partial` to .


Diffs
-

  3rdparty/stout/include/stout/lambda.hpp 
a61d97b69e7eebd057c94148d39c6e1fc3066017 
  3rdparty/stout/tests/lambda_tests.cpp 
ad8c2efddb6b64184670d0cfb33188ef843351ab 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 63636: Added placeholder implementation.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/lambda.hpp
Lines 474-476 (patched)
<https://reviews.apache.org/r/63636/#comment270717>

We should inherit from `lambda::Placeholder` here,
`is_placeholder` is expected to provide `::value` for example.



3rdparty/stout/include/stout/lambda.hpp
Lines 479-481 (patched)
<https://reviews.apache.org/r/63636/#comment270713>

We shouldn't need this one.


- Michael Park


On Nov. 22, 2017, 6:13 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63636/
> ---
> 
> (Updated Nov. 22, 2017, 6:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added placeholder implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/lambda.hpp 
> a61d97b69e7eebd057c94148d39c6e1fc3066017 
> 
> 
> Diff: https://reviews.apache.org/r/63636/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63632: Migrated to event consumer interface.

[Michael Park]

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


Fix it, then Ship it!





src/master/master.hpp
Line 548 (original), 548 (patched)
<https://reviews.apache.org/r/63632/#comment270705>

Can we remove the `virtual`s here similar to the libprocess patch?



src/master/master.cpp
Lines 1619 (patched)
<https://reviews.apache.org/r/63632/#comment270708>

I think the `TODO` is sufficient as breadcrumb
for us to know that we need to change code here.

No need to make an extra copy of `MessageEvent`,
here and below.


- Michael Park


On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63632/
> ---
> 
> (Updated Nov. 7, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrated to event consumer interface.
> 
> 
> Diffs
> -
> 
>   src/common/recordio.hpp 378363492e04c141671fbed0467bf558b61e5dae 
>   src/master/master.hpp 0c1253aa1a88d113d42ce362d6992ad84217728c 
>   src/master/master.cpp ee212c1a21b432d3aa8e90d748d12cb0a754d57c 
>   src/tests/fetcher_cache_tests.cpp e2ecb102e2e7b4c4b3b2b736fdc453feeb15816e 
> 
> 
> Diff: https://reviews.apache.org/r/63632/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63631: Separated event visiting and consumption.

[Michael Park]

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


Fix it, then Ship it!




The `ProcessBase` and `EventVisitor` relationship is a bit weird already,
and I'm a bit concerned about essentially changing the definition of
`ProcessBase` across the entire codebase, but I think overall the patch
looks consistent and correct.


3rdparty/libprocess/src/tests/benchmarks.cpp
Lines 608-609 (original), 608-609 (patched)
<https://reviews.apache.org/r/63631/#comment270702>

Let's remove this since we're constructing new ones within the `for` loop 
now.


- Michael Park


On Nov. 7, 2017, 8:51 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63631/
> ---
> 
> (Updated Nov. 7, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This introduces `EventConsumer` interface, which add support of events
> with movable only data. This allows consumers to move data out of
> event, rather than copying it. This is required to implement movable
> only objects support in `defer` to guarantee that deferred functor is
> invoked only once, allowing deferred parameters to be moved into call.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/event.hpp 
> 2e246205020c3c5b8c2eb5187a8eb3643d1e6d4d 
>   3rdparty/libprocess/include/process/process.hpp 
> dc3375ce62556322eb2bc60ade61f313ade123b8 
>   3rdparty/libprocess/include/process/protobuf.hpp 
> fe152f273332470ac50f9715291897bb04cf95b9 
>   3rdparty/libprocess/include/process/run.hpp 
> 1540a78e52a90d4c1d4165c46be353caaad21bce 
>   3rdparty/libprocess/src/process.cpp 
> 71ae7129ffbd0e22eda2863b17bbcf588298c37b 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> e6c77d565d5acf72b475a085e9504679253b4b97 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 952c92c033e2363cff0c2c68610d3820b97d177e 
> 
> 
> Diff: https://reviews.apache.org/r/63631/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 63630: Added support for callable once functors.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/lambda.hpp
Lines 357 (patched)
<https://reviews.apache.org/r/63630/#comment270696>

Let's use `cpp17::invoke` here.



3rdparty/stout/include/stout/lambda.hpp
Lines 375 (patched)
<https://reviews.apache.org/r/63630/#comment270697>

Let's elaborate on this comment a little bit around
the heap allocation involved here due to
the type-erasure and mention that `std::function`
also has this overhead-ish.



3rdparty/stout/include/stout/lambda.hpp
Lines 393 (patched)
<https://reviews.apache.org/r/63630/#comment270693>

Let's decay this earlier.



3rdparty/stout/include/stout/lambda.hpp
Lines 404 (patched)
<https://reviews.apache.org/r/63630/#comment270695>

Can we make this a `unique_ptr` such that we can default some of the 
operations?



3rdparty/stout/include/stout/lambda.hpp
Lines 420-425 (patched)
<https://reviews.apache.org/r/63630/#comment270694>

We recently started to stick to this pattern:
```
tmeplate ::type = 0>
```


- Michael Park


On Nov. 22, 2017, 6:10 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63630/
> ---
> 
> (Updated Nov. 22, 2017, 6:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `CallableOnce` class is similar to `std::function`, but allows it to be
> called only once. Together with `partial` this provides foundation for
> copy-less `defer`, `dispatch` and `Future`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/lambda.hpp 
> a61d97b69e7eebd057c94148d39c6e1fc3066017 
> 
> 
> Diff: https://reviews.apache.org/r/63630/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Review Request 64248: Added `cpp17::invoke` in .

[Michael Park]

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

Review request for mesos and Dmitry Zhuk.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/stout/Makefile.am 4386017acd6ca465be3f735460c11d50b440ccc5 
  3rdparty/stout/include/stout/cpp17.hpp PRE-CREATION 
  3rdparty/stout/tests/CMakeLists.txt 6e5773f1e03671de7ac007caf49edd0f1cd7aedd 
  3rdparty/stout/tests/cpp17_tests.cpp PRE-CREATION 


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


Testing
---

Addded `cpp17_tests.cpp`


Thanks,

Michael Park

Re: Review Request 63628: Implemented index_sequence and related functionality.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Nov. 30, 2017, 12:40 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63628/
> ---
> 
> (Updated Nov. 30, 2017, 12:40 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-6972
> https://issues.apache.org/jira/browse/MESOS-6972
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds implementation of C++14 STL's `index_squence`
> and `make_index_sequence` templates.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/cpp14.hpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/63628/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 64195: Updated the tests to use MULTI_ROLE frameworks by default.

[Michael Park]

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


Ship it!





src/tests/persistent_volume_tests.cpp
Line 150 (original), 152 (patched)
<https://reviews.apache.org/r/64195/#comment270565>

This is to get a stable result from the `join` below, right?



src/tests/slave_tests.cpp
Line 860 (original), 860 (patched)
<https://reviews.apache.org/r/64195/#comment270562>

Huh... so these used to be an empty string?


- Michael Park


On Nov. 29, 2017, 5:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64195/
> ---
> 
> (Updated Nov. 29, 2017, 5:45 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-8237
> https://issues.apache.org/jira/browse/MESOS-8237
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Now that we strip the `Resource.allocation_info` for non-MULTI_ROLE
> schedulers, it's simpler to default the tests to use the MULTI_ROLE
> capability, since we've already updated the majority of the tests
> to be aware of `Resource.allocation_info`.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp 0136344c160ea6aeacc57e61382640689a77ee1a 
>   src/examples/disk_full_framework.cpp 
> 28f71c35b32b46c7bf2cce9fe971266f7bd73c8a 
>   src/examples/dynamic_reservation_framework.cpp 
> 5ee3867228a70c8fa0b4b5092fb44cb556a670e0 
>   src/examples/long_lived_framework.cpp 
> e6742159108f9eae4f2548bb84d6495309737d9c 
>   src/examples/no_executor_framework.cpp 
> fd920f58f5ccb903127e9c6b112be30f04baa069 
>   src/examples/persistent_volume_framework.cpp 
> 674d58a9e6fa34733f765d2925cedafeaab1eecd 
>   src/examples/test_framework.cpp c6a293ed5f88bf4d73f23d97cbf9ffdd8b941f1a 
>   src/examples/test_http_framework.cpp 
> 373035837f51e818629b1588d2ab8803c656f527 
>   src/tests/api_tests.cpp 66cb059c96ff9ecda594e13de9e5dc3909f3 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> 421a72f1694a1a5c5e1bf4c525361dbd670c68af 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 7a42bb93508abf9020d0bcc5b3daa2f6834be2e3 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 655f9f81f4bc55a85d7c5e27609a016254793380 
>   src/tests/default_executor_tests.cpp 
> 04d9e1b9d4682c3bb5355253e3a8e5b06d78e98f 
>   src/tests/disk_quota_tests.cpp fc297995c42821aa4b0a7719a6a19c3bbf65db47 
>   src/tests/fault_tolerance_tests.cpp 
> 33a2220b4b4f019799db7b2482ce73b3454fa58e 
>   src/tests/hook_tests.cpp 2e58d1133343a96b67d0816a54ff8eb874522ba3 
>   src/tests/master_allocator_tests.cpp 
> 3400d70bb0ba564eac43c4639eee0efd4d8059e6 
>   src/tests/master_authorization_tests.cpp 
> eff97f155e7472a0cd5994408ed73474392593ad 
>   src/tests/master_quota_tests.cpp 058f6d24da50cbf3c28b091afa88f634a8102b62 
>   src/tests/master_tests.cpp 01f45a9bb6378c5fded31bf58fbcad93fe7ed719 
>   src/tests/master_validation_tests.cpp 
> 0e1c8b490ebe10bc50b8b6b530fa85128b967584 
>   src/tests/mesos.hpp f02c7c6962c9d0bee57712d8aad9997582c7404b 
>   src/tests/oversubscription_tests.cpp 
> 2f403d3c9d2df7d5425b335781e8b62153854ef3 
>   src/tests/partition_tests.cpp e49c474167076b4136a161ed29b11db9a13455a7 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 883192d2badf8af87c95bc9a5fc3e711cd240193 
>   src/tests/persistent_volume_tests.cpp 
> b11d26e45c184cf3f0623629b898f67dc95c4fa0 
>   src/tests/reservation_endpoints_tests.cpp 
> a96cc714fd40421a7db9a4c0debf71546daf5d4c 
>   src/tests/reservation_tests.cpp fa375cd45e4c68551801521176632a56acd6d0ea 
>   src/tests/resource_provider_manager_tests.cpp 
> 0b7c4ad6bb0052847b884959e3171cd7ab382b45 
>   src/tests/role_tests.cpp 084555ae70b80330cdc8729cd62f8baddf291a01 
>   src/tests/scheduler_http_api_tests.cpp 
> 80e52fb22dbba4a0703f1b192205acaff89f883b 
>   src/tests/scheduler_tests.cpp 45fc9c0cfccdb22c2e3e8d5de30c04575814a0e9 
>   src/tests/slave_authorization_tests.cpp 
> 11fd0d4e35523eca23a9603ecef0c9d0b65dde38 
>   src/tests/slave_recovery_tests.cpp f14c6ef69eb20a03454c8197df79b572a3c6d050 
>   src/tests/slave_tests.cpp a2274b691cf94b003c4bc15450d176a9c73517d5 
>   src/tests/upgrade_tests.cpp 7f434dbba858f636719eec24e92b306b76430c4c 
> 
> 
> Diff: https://reviews.apache.org/r/64195/diff/1/
> 
> 
> Testing
> ---
> 
> Ran the tests, including with root.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 64194: Stripped AllocationInfo from offers to non-MULTI_ROLE schedulers.

[Michael Park]

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


Ship it!





src/master/master.cpp
Lines 8128 (patched)
<https://reviews.apache.org/r/64194/#comment270541>

`s/"old"/pre-MULTI_ROLE/`?



src/master/master.cpp
Lines 8129-8136 (patched)
<https://reviews.apache.org/r/64194/#comment270547>

I found this example a bit difficult to follow...
Maybe it's enough to link the JIRA and leave a simple
comment saying:

> "pre-MULTI_ROLE" schedulers are not `AllocationInfo`
> aware, and since they may be performing operations
> that implicitly uses all of `Resource`'s state
> (e.g., equality comparison), we strip
> the `AllocationInfo` from `Resource`, as well as
> `Offer`.



src/master/master.cpp
Lines 8129-8139 (patched)
<https://reviews.apache.org/r/64194/#comment270548>

I found this example a bit difficult to follow...
Maybe it's enough to link the JIRA and leave a simple
comment saying:

> "pre-MULTI_ROLE" schedulers are not `AllocationInfo`
> aware, and since they may be performing operations
> that implicitly uses all of `Resource`'s state
> (e.g., equality comparison), we strip
> the `AllocationInfo` from `Resource`, as well as
> `Offer`.



src/master/master.cpp
Lines 8143 (patched)
<https://reviews.apache.org/r/64194/#comment270542>

`s/r/resource/`?


- Michael Park


On Nov. 29, 2017, 5:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64194/
> -------
> 
> (Updated Nov. 29, 2017, 5:45 p.m.)
> 
> 
> Review request for mesos, James DeFelice and Michael Park.
> 
> 
> Bugs: MESOS-8237
> https://issues.apache.org/jira/browse/MESOS-8237
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per MESOS-8237, it is problematic to show `Resource.allocation_info`
> for "old" schedulers. The example that was presented was the
> "mesos-go" 3rd party library that provides resource comparison logic.
> The library has both MULTI-ROLE and non-MULTI_ROLE schedulers as
> clients, these clients use the comparison logic supplied by the
> library to see if the offer contains the resources they need.
> The library author does not want to have to do any stripping of the
> allocation info to preserve the equality.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 700e12433b0b66efc3f5dd296711c0f203a13144 
> 
> 
> Diff: https://reviews.apache.org/r/64194/diff/1/
> 
> 
> Testing
> ---
> 
> Updated the tests in the subsequent review.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>