Re: Review Request 36908: Added QuotaInfo Protobuf.

[Qian Zhang]

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

Ship it!


Ship It!

- Qian Zhang


On Sept. 2, 2015, 9:38 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36908/
> ---
> 
> (Updated Sept. 2, 2015, 9:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-3164
> https://issues.apache.org/jira/browse/MESOS-3164
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added QuotaInfo Protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/quota.hpp PRE-CREATION 
>   include/mesos/master/quota.proto PRE-CREATION 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
> 
> Diff: https://reviews.apache.org/r/36908/diff/
> 
> 
> Testing
> ---
> 
> make distcheck
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 38171: Stout: Refactored set to use initializer list for variadic constructor.

[Joris Van Remoortere]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/set.hpp 
85466dbb36b3ac545562eafe8041ad79993fdf9f 

Diff: https://reviews.apache.org/r/38171/diff/


Testing
---


Thanks,

Joris Van Remoortere

Review Request 38172: Stout: Simplified hashset implementation.

[Joris Van Remoortere]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 
1839d28638cd82dae10ba9b0f99c1a97cf34f9c9 

Diff: https://reviews.apache.org/r/38172/diff/


Testing
---


Thanks,

Joris Van Remoortere

Review Request 38173: Stout: Simplified hashmap implementation.

[Joris Van Remoortere]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 
f09bea125035aa3621402b83608b233e42877559 

Diff: https://reviews.apache.org/r/38173/diff/


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 36913: Added /quota HTTP Endpoint for Quota handling.

[Qian Zhang]

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



src/master/master.hpp (line 901)


Why we add CALL_HELP back?


- Qian Zhang


On Sept. 3, 2015, 12:16 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36913/
> ---
> 
> (Updated Sept. 3, 2015, 12:16 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> Bugs: MESOS-3073
> https://issues.apache.org/jira/browse/MESOS-3073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added /quota HTTP Endpoint for Quota handling.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/master/http.cpp 94e97a2898106579434e8cdec04b7b0e130a810e 
>   src/master/master.hpp 594dd25f9aa9b6147680d0a838a77c3222941f4b 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
>   src/master/quota_handler.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/36913/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37714: Updated Multimap and multihashmap so their signatures resemble that of hashmap and hashset.

[Bernd Mathiske]

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

Ship it!


Ship It!

- Bernd Mathiske


On Aug. 28, 2015, 1:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37714/
> ---
> 
> (Updated Aug. 28, 2015, 1:11 a.m.)
> 
> 
> Review request for mesos, Joerg Schad, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-2924
> https://issues.apache.org/jira/browse/MESOS-2924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra template parameters to `multihashmap` which offer control over the 
> hash function to use as well as the equality operator.
> 
> Implements initializer_list, copy and move constructors for both, 
> `multihashmap` and `Multimap` in a similar way as it was done for `hashmap` 
> and `hashset`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
> d9e4031cee64e48ad50541c04ca11e7861d0a17c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp 
> fb3e7a1d0377001389980302342813217f49cf5f 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 
> 535cd2d10e3074c86c149ce85b205e73ca42ddd3 
> 
> Diff: https://reviews.apache.org/r/37714/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Guangya Liu]

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

(Updated 九月 8, 2015, 6:16 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


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


Repository: mesos


Description
---

Replace hard-coded reap interval with a constant


Diffs (updated)
-

  src/tests/containerizer/launch_tests.cpp 
d211fc0f665988068c67836ef80916828a0df2bd 
  src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
  src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
  src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 

Diff: https://reviews.apache.org/r/38046/diff/


Testing
---


Thanks,

Guangya Liu

Re: Review Request 38046: Replace hard-coded reap interval with a constant

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38046]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 6:16 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38046/
> ---
> 
> (Updated Sept. 8, 2015, 6:16 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-1935
> https://issues.apache.org/jira/browse/MESOS-1935
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace hard-coded reap interval with a constant
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launch_tests.cpp 
> d211fc0f665988068c67836ef80916828a0df2bd 
>   src/tests/gc_tests.cpp ec27ac7f1fca7f425abcea0eafaac4fae905fb8f 
>   src/tests/slave_recovery_tests.cpp 6aae14a3c39b9aee76147b691b0170946e1120b5 
>   src/tests/slave_tests.cpp 5c1a3d36a5f67629aef275eeae12956c524e8102 
> 
> Diff: https://reviews.apache.org/r/38046/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37714: Updated Multimap and multihashmap so their signatures resemble that of hashmap and hashset.

[Bernd Mathiske]

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



3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp (line 31)


I am grateful that you do not use single letter type names here! Thank you! 
Following your example would greatly improve readability in so many other 
places.


- Bernd Mathiske


On Aug. 28, 2015, 1:11 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37714/
> ---
> 
> (Updated Aug. 28, 2015, 1:11 a.m.)
> 
> 
> Review request for mesos, Joerg Schad, Michael Park, and Jan Schlicht.
> 
> 
> Bugs: MESOS-2924
> https://issues.apache.org/jira/browse/MESOS-2924
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds extra template parameters to `multihashmap` which offer control over the 
> hash function to use as well as the equality operator.
> 
> Implements initializer_list, copy and move constructors for both, 
> `multihashmap` and `Multimap` in a similar way as it was done for `hashmap` 
> and `hashset`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multihashmap.hpp 
> d9e4031cee64e48ad50541c04ca11e7861d0a17c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/multimap.hpp 
> fb3e7a1d0377001389980302342813217f49cf5f 
>   3rdparty/libprocess/3rdparty/stout/tests/multimap_tests.cpp 
> 535cd2d10e3074c86c149ce85b205e73ca42ddd3 
> 
> Diff: https://reviews.apache.org/r/37714/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Jojy Varghese]

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



src/slave/containerizer/provisioner.hpp (line 54)


doxygen style?



src/slave/containerizer/provisioner.cpp (line 36)


I think we should guard against multiple "create" calls. Call to "create" 
multiple times here would repeat the whole logic of creating the creator map 
and provsioners.



src/slave/containerizer/provisioners/docker.hpp (line 56)


doxygen comments? This applies for other places too.



src/slave/containerizer/provisioners/docker.hpp (line 65)


s/repo/repositorypath



src/slave/containerizer/provisioners/docker.hpp (line 67)


I believe the default value of an option is None.So need not explicitly 
declare.



src/slave/containerizer/provisioners/docker.hpp (line 94)


Is it layers or just layer ids? If its just layer ids, then the struct 
should be DockerImageInfo (and not DockerImage).



src/slave/containerizer/provisioners/docker.hpp (line 121)


According to google style guide, this should be declared last. 

Also, why not use the "delete" keyword (c++11)?



src/slave/containerizer/provisioners/docker.cpp (line 37)


maybe explicit namespace names?



src/slave/containerizer/provisioners/docker.cpp (line 56)


I strongly believe that we should have strong/explicit ownership semantics 
for pointers (instead of raw pointers).



src/slave/containerizer/provisioners/docker.cpp (line 89)


Would be better if we follow the factory patttern (create method that 
returns a Try)?



src/slave/containerizer/provisioners/docker.cpp (line 111)


should we create a guard here so that multiple calls to "create" wont call 
multiple "process" create?



src/slave/containerizer/provisioners/docker.cpp (line 172)


should we check/validate for the docker_rootfs_dir?



src/slave/containerizer/provisioners/docker.cpp (line 183)


const?



src/slave/containerizer/provisioners/docker.cpp (line 279)


since destroy can be called from multiple thread contexts, should we 
proetct it?



src/slave/containerizer/provisioners/docker/local_store.hpp (line 34)


destructor declaration should be after constructor(create)?



src/slave/containerizer/provisioners/docker/local_store.hpp (line 53)


This should be  declared last. Applies everywhere.



src/slave/containerizer/provisioners/docker/local_store.cpp (line 94)


s/refStore/referenceStore?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 102)


guard for multiple calls?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 157)


here it is possible that refStore could be not created and we still have a 
LocalStoreProcess object. Maybe we should move the refStore creation to before 
line 152.



src/slave/containerizer/provisioners/docker/local_store.cpp (line 167)


const?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 174)


Can this return an error?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 179)


Can this return an error?



src/slave/containerizer/provisioners/docker/local_store.cpp (line 280)


s/layers/layerIds ?



src/slave/flags.hpp (line 56)


Adding more fields to base Flag class might not scale? Should we adopt 
feature/subsystem specific flags as subclass of base Flags?


- Jojy Varghese


On Sept. 7, 2015, 11:31 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> 

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

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

(Updated Sept. 8, 2015, 7:52 p.m.)


Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.


Repository: mesos


Description
---

Joining all the commits around provisioner and local store into one review so 
it's easier to review, as patches
are changing code on top of each other.

All the commits are going to committed together.


Diffs (updated)
-

  src/Makefile.am 5fdca0f 
  src/messages/docker_provisioner.hpp PRE-CREATION 
  src/messages/docker_provisioner.proto PRE-CREATION 
  src/slave/containerizer/provisioner.hpp 9e0e0b8 
  src/slave/containerizer/provisioner.cpp 95894c0 
  src/slave/containerizer/provisioners/appc.cpp fc5ee19 
  src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
  src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
  src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/reference_store.cpp PRE-CREATION 
  src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
  src/slave/flags.hpp b8335aa 
  src/slave/flags.cpp 7539441 
  src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/38137/diff/


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 8, 2015, 7:25 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Changes
---

Added missing dot at end of comment.


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


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

[Wojciech Sielski]

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

(Updated Sept. 8, 2015, 7:46 p.m.)


Review request for mesos and Timothy Chen.


Changes
---

Rename for MESOS_CONTAINER_NAME


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


Repository: mesos


Description
---

MESOS-3377 - Adding CONTAINER_NAME as additional env variable


Diffs (updated)
-

  src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 

Diff: https://reviews.apache.org/r/38165/diff/


Testing
---

Every docker container that is created by mesos,
gonna get conatiner dynamic env variable CONTAINER_NAME.
So it is easy to use by 3rd party application,
for example: when use with registartor/consul 
so is it's much easier to deregister itself
(or put into maintanance mode) before being killed.


Tested and verified afetr build locally.


Thanks,

Wojciech Sielski

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 7:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 7:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38143: Adding executor HTTP API protobuf to V1

[Anand Mazumdar]

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

Ship it!


LGTM !

@gyliu : This change just moves the existing executor protobuf's to the V1 
namespace, we can revisit the semantics later when we finalize the design doc. 
Also, thanks for pointing out the typos we had missed during the earlier review 
of these protobuf's.
@ijimenez : Can we delete the old executor protobuf's/headers in a separate 
patch ? I am assuming that no one is using them.

- Anand Mazumdar


On Sept. 5, 2015, 10:05 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38143/
> ---
> 
> (Updated Sept. 5, 2015, 10:05 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3375
> https://issues.apache.org/jira/browse/MESOS-3375
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Executor protobufswere introduced in Mesos for the HTTP API, they need to be 
> added to /v1 so they reflect changes made on v1/mesos.proto. This protobuf 
> ought to be changed as the executor HTTP API design evolves.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/executor/executor.hpp PRE-CREATION 
>   include/mesos/v1/executor/executor.proto PRE-CREATION 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38143/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 8, 2015, 7:22 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


Changes
---

Improved the deleting of ranges in coalesce(ranges).


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


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 37703: Add docker exec command.

[Timothy Chen]

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



src/docker/docker.cpp (line 1200)


4 space indent instead of 8



src/tests/containerizer/docker_tests.cpp (line 241)


Please follow how other tests are testing preconditions, one example:

ASSERT_SOME(
  cgroups::memory::oom::killer::enabled(hierarchy, TEST_CGROUPS_ROOT))
<< "-\n"
<< "We cannot run this test because it appears you do not have\n"
<< "a modern enough version of the Linux kernel. You won't be\n"
<< "able to use the cgroups isolator, but feel free to disable\n"
<< "this test.\n"
<< "-";

And also move this to the beginning of the test


- Timothy Chen


On Aug. 30, 2015, 8:16 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37703/
> ---
> 
> (Updated Aug. 30, 2015, 8:16 a.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3291
> https://issues.apache.org/jira/browse/MESOS-3291
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add docker exec command.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 6086710fff32a25e46197a69ae1063074317221b 
>   src/docker/docker.cpp 12dc0505c9ec4bd380e817d44da2c4e8d1b0d5f5 
>   src/tests/containerizer/docker_tests.cpp 
> a4a2725c05ae0cb88426c587f7ded0da77154edc 
> 
> Diff: https://reviews.apache.org/r/37703/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

[Timothy Chen]

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



src/docker/docker.cpp (line 414)


Let's prefix this with MESOS_ (MESOS_CONTAINER_NAME).

Otherwise this looks good to me.


- Timothy Chen


On Sept. 8, 2015, 6:17 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated Sept. 8, 2015, 6:17 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, line 231
> > 
> >
> > An idea to re-use code here. This function boils down to inserting an 
> > element into a sorted range and then performing one step of coalescing from 
> > this element till the end of the sequence. We can keep the insetion 
> > "virual" and move the elements once during the second coalescing pass (see 
> > my comment above).

New code does only insert a new range if not overlapping/neighboring with 
existing ranges, otherwise existing ranges are updated.


- Joerg


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


On Sept. 8, 2015, 7:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 7:25 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38173: Stout: Simplified hashmap implementation.

[Joris Van Remoortere]

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

(Updated Sept. 8, 2015, 9:52 p.m.)


Review request for mesos and Michael Park.


Changes
---

updated chain.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 
f09bea125035aa3621402b83608b233e42877559 

Diff: https://reviews.apache.org/r/38173/diff/


Testing
---

make check


Thanks,

Joris Van Remoortere

Re: Review Request 38117: Export per container SNMP statistics

[Cong Wang]

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

(Updated Sept. 8, 2015, 9:12 p.m.)


Review request for mesos, Jie Yu, Paul Brett, and Vinod Kone.


Changes
---

Address review comment


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


Repository: mesos


Description
---

These stats are those we get by `netstat -s`, they are important for diagnose 
networking issues.


Diffs (updated)
-

  include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
  src/slave/containerizer/isolators/network/port_mapping.hpp 
4bca0b81bf69fb4cd75e05aacd02d3e818e32d09 
  src/slave/containerizer/isolators/network/port_mapping.cpp 
34ba2294b0bd7d57aa9de073692a2ea8ec62681d 
  src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
  src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d 

Diff: https://reviews.apache.org/r/38117/diff/


Testing
---

Manual tests


Thanks,

Cong Wang

Review Request 38191: Removing unused Executor protobuf

[Isabel Jimenez]

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

Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.


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


Repository: mesos


Description
---

The executor protobuf definition living outside the v1/ directory is unused, it 
should be removed to avoid confusion.


Diffs
-

  include/mesos/executor/executor.hpp 85f181c 
  include/mesos/executor/executor.proto 52c84b3 
  src/Makefile.am 5fdca0f 

Diff: https://reviews.apache.org/r/38191/diff/


Testing
---

make check


Thanks,

Isabel Jimenez

Re: Review Request 38154: Switched to type traits for checking whether a type is a protobuf message.

[Joseph Wu]

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

Ship it!


One minor nit.


3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp (lines 557 - 558)


I think this comment isn't really necessary.  You could remove it.

And the comment adds a bit of ambiguity (i.e. what's different about 
`is_convertible` that `is_public_base_of` provides?)


- Joseph Wu


On Sept. 7, 2015, 4:24 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38154/
> ---
> 
> (Updated Sept. 7, 2015, 4:24 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3312
> https://issues.apache.org/jira/browse/MESOS-3312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/protobuf.hpp 
> 57d5fdf45273c620655b44b5f5572290cffa4bf6 
> 
> Diff: https://reviews.apache.org/r/38154/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38172: Stout: Simplified hashset implementation.

[Joris Van Remoortere]

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

(Updated Sept. 8, 2015, 9:52 p.m.)


Review request for mesos and Michael Park.


Changes
---

fixed ambiguity that failed gcc-4.8


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/hashset.hpp 
1839d28638cd82dae10ba9b0f99c1a97cf34f9c9 

Diff: https://reviews.apache.org/r/38172/diff/


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 38137: Added Docker provisioner and local store

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38137]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 7:52 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 8, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   src/slave/containerizer/provisioner.hpp 9e0e0b8 
>   src/slave/containerizer/provisioner.cpp 95894c0 
>   src/slave/containerizer/provisioners/appc.cpp fc5ee19 
>   src/slave/containerizer/provisioners/appc/paths.hpp fb3a1a7 
>   src/slave/containerizer/provisioners/appc/paths.cpp e6be851 
>   src/slave/containerizer/provisioners/docker.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/local_store.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/paths.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.hpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/reference_store.cpp 
> PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/store.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
>   src/slave/flags.hpp b8335aa 
>   src/slave/flags.cpp 7539441 
>   src/tests/containerizer/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38137/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38165]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 7:46 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated Sept. 8, 2015, 7:46 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in libprocess.

[Joseph Wu]

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

Ship it!


Just a bit more cleanup, and I think this would be good to go.

If possible, you should coordinate with Alex Clemmer to make sure this change 
doesn't conflict/break what he's working on 
(https://reviews.apache.org/r/37019/).


3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (lines 19 - 21)


Not sure if these comments are necessary.

If anything, just keep one:
```
:: Path to the solution (.sln) file being built.
set SOLUTION_FILE=%-1
```



3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (lines 23 - 24)


Put a newline before the comment.

Suggestion:
```

:: List of project names delimited by '#'.
```



3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 42)


I think you don't need this anymore.



3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 48)


Not necessary anymore.



3rdparty/libprocess/cmake/macros/VsBuildCommand.bat (line 52)


Not used.


- Joseph Wu


On Sept. 5, 2015, 5:43 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> ---
> 
> (Updated Sept. 5, 2015, 5:43 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake macro VsBuildCommand in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> 997cc0d0e316e316136d4746e50e9e292a82b36b 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 12506a1369de005285268f895f365aba0c560f78 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 
>   3rdparty/libprocess/cmake/macros/Noop.cmake PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37273/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

[Timothy Chen]

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

Ship it!


Ship It!

- Timothy Chen


On Sept. 8, 2015, 7:46 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated Sept. 8, 2015, 7:46 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 38161: Replaced a hard-coded number for registration backoff with a proper constant and extended comments.

[Bernd Mathiske]

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



src/tests/fault_tolerance_tests.cpp (line 675)


Please clarify that this is a side effect of the following statement.

"As a side-effect of the following, ..."



src/tests/fault_tolerance_tests.cpp (line 705)


s/re-/
s/retry/



src/tests/fault_tolerance_tests.cpp (line 1545)


s/a default/a known value instead of what CreateSlaveFlags() or 
parameterless StartSlave() would assign.



src/tests/fault_tolerance_tests.cpp (line 1572)


s/re-/
s/retry/


- Bernd Mathiske


On Sept. 7, 2015, 9:04 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38161/
> ---
> 
> (Updated Sept. 7, 2015, 9:04 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/fault_tolerance_tests.cpp 
> 89cb18be96cd60fb77fbcc4acd08cebdcf1ba075 
> 
> Diff: https://reviews.apache.org/r/38161/diff/
> 
> 
> Testing
> ---
> 
> make check (Mac OS 10.10.4)
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 277-278
> > 
> >
> > Since you're editing here, mind explaining what an "urange" is?

As the comment above states it is an Un-coalesced range, will try to highlight 
that more.


> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, line 665
> > 
> >
> > Just to make sure, you have added just `2` lines to this function (not 
> > easy on RB):
> > ```
> >   sort(ranges);
> >   coalesce(ranges);
> > ```
> > Correct?

And removed one temporary Ranges object (see line 72 in previous version).


> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 442-444
> > 
> >
> > Let's let compiler do it's job: how about passing `left` by value and 
> > not creating a copy manually?

Wanted to keep the (internal) interface consistent with the other operators. Do 
you have a strong preference here?


> On Sept. 7, 2015, 9:24 p.m., Alexander Rukletsov wrote:
> > src/common/values.cpp, lines 185-186
> > 
> >
> > s/is/are
> > 
> > I would suggest we write high level comments. How about this: "We 
> > assume ranges are sorted in ascending order of the lower endpoint."

ranges is the Ranges object i.e. is, and otherwise I personally find 
range.begin() a more precise description


- Joerg


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


On Sept. 7, 2015, 6:04 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 7, 2015, 6:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 37336: Simplified the caller interface to process::Subprocess

[Marco Massenzio]

> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 103-110
> > 
> >
> > Does it make sense to aggregate these into a `Result`? The 
> > `Some` case would be stdout, the `Error` case would be stederr, and the 
> > `None` case would represent not known yet?
> > If you don't need the `None` case then you can use a `Try`.

I like the idea and it would be a very elegant solution; unfortunately, I 
suspect it may not work in the more general case.

In my experience, Linux processes emit to both stdout and stderr (sometimes, 
even though there may be no error -- eg `rsync` and `tar` do, and, for 
non-fatal errors, they happily carry on; and can even be "forced" to continue 
in the presence of errors) - so, sometimes, you need both.

Also, bearing in mind there may be a bunch of output on `stdout` *before* the 
error, which then manifests itself in `stderr`: but one really needs to look at 
both to really do discovery.

Does it make sense?


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/subprocess.hpp, lines 350-354
> > 
> >
> > The indentation is a little different from your comment in 
> > `subprocess.hpp`. Let's be consistent between them, ideally also with the 
> > rest of the codebase if you can find some good examples.

so, this is `subprocess.hpp` and having looked at all of them, I notice that 
the *NOTE* pattern is not used; so I've changed it to `NOTE:`
However, they all seem aligned the same way (no indent).

I'll keep an eye for other places in my diff that may have different 
indentantion and will fix them too.


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 197
> > 
> >
> > I don't think it hurts to take timeout by const ref here. It helps the 
> > reader understand you don't intend to copy and modify it.
> > `const Duration& timeout`.
> > I acknowledge your have knowledge of the internal layout of the 
> > datastructure and know it's equally cheap to copy it. If someone came along 
> > in the future and added to the internal state of `Duration` then they 
> > wouldn't have to refactor your code :-)

You're being generous here :)
(but, really, can't think of a good reason why I didn't use a const & in the 
first place - it's my go-to choice...)
Fixed!


> On Sept. 7, 2015, 11:14 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, line 199
> > 
> >
> > I believe the issues you are running into regarding `FIXME(marco)` are 
> > rooted in your promise not having a future associated with it.
> > 
> > Usually the patter we follow is:
> > ```
> > // Create a promise.
> > Promise* promise = new promise();
> > 
> > // Get the future from the promise.
> > Future future = promise->future();
> > 
> > // Attach any mandatory chained functions to the future.
> > future.then([](){ ...; });
> > 
> > // Schedule our async action, and fulfill the promise inside that 
> > action.
> > io::read(fd).then([=]() {
> >   promise->set(...);
> > }).onFailed([=](const std::string& err) {
> >   promise->fail(err);
> > });
> > 
> > // Return the future to the user for them to attach any of their own 
> > callbacks.
> > return future.
> > ```
> > 
> > Feel free to sync with MPark or me to understand this further offline.

right - that's the pattern I had actually used in my preliminary tests; and it 
worked.
What throws a spanner in the works are a couple of things:

1) the Future returned from `await()` - not sure what to do with it:
```
  // We need to wait on the process to complete, as well as for
  // stdout and stderr to be available.
  Future, Future, Future>> waitRes =
  await(status(), stdout(), stderr());
```

2) returning a `Future` to the caller, instead of waiting inside the `wait()` 
method makes the caller API ugly:
```
Future future = subprocess.wait();
await(future, Seconds(5));
```
or something along those lines - I wanted to actually "compress" the two calls 
into one.

The question I had was more along the lines: given the current code (and the 
fact that it seems to "work as intended") would you be happy as to where it 
currently stands? or am I missing something that will come back and bite us?

Thanks!


- Marco


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


On Aug. 

Review Request 38193: Remove comment from updateAllocation

[Guangya Liu]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

The allocation for updateAllocation will always be updated after
reserve, unreserve, create and destroy, so there is no need to
add the TODO comments of "Do not log if there is no update".


Diffs
-

  src/master/allocator/mesos/hierarchical.hpp 
cb4020dea897ef198cd9898cabecf61edfade834 

Diff: https://reviews.apache.org/r/38193/diff/


Testing
---


Thanks,

Guangya Liu

Re: Review Request 38191: Removing unused Executor protobuf

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 九月 8, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38191/
> ---
> 
> (Updated 九月 8, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3393
> https://issues.apache.org/jira/browse/MESOS-3393
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor protobuf definition living outside the v1/ directory is unused, 
> it should be removed to avoid confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor/executor.hpp 85f181c 
>   include/mesos/executor/executor.proto 52c84b3 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 38117: Export per container SNMP statistics

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38117]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 9:12 p.m., Cong Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38117/
> ---
> 
> (Updated Sept. 8, 2015, 9:12 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Paul Brett, and Vinod Kone.
> 
> 
> Bugs: MESOS-3365
> https://issues.apache.org/jira/browse/MESOS-3365
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These stats are those we get by `netstat -s`, they are important for diagnose 
> networking issues.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto b1deed4720cab4a89db76a48bc9563bba4f5bf1c 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 
> 4bca0b81bf69fb4cd75e05aacd02d3e818e32d09 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 34ba2294b0bd7d57aa9de073692a2ea8ec62681d 
>   src/slave/flags.hpp b8335aa585376d27b71897f8cbaefaa12f4b3a5c 
>   src/slave/flags.cpp 7539441c685828027db07173e62a4e5fc1e8b54d 
> 
> Diff: https://reviews.apache.org/r/38117/diff/
> 
> 
> Testing
> ---
> 
> Manual tests
> 
> 
> Thanks,
> 
> Cong Wang
> 
>

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

[Guangya Liu]

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

Ship it!


Ship It!

- Guangya Liu


On 九月 8, 2015, 7:46 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated 九月 8, 2015, 7:46 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Guangya Liu]

> On 九月 8, 2015, 3:44 p.m., Guangya Liu wrote:
> > docs/mesos-testing-patterns.md, line 10
> > 
> >
> > What does "Naïve" means?
> 
> haosdent huang wrote:
> Naïve waiting means just waiting until timeout. Like:
> ```
> AWAIT_READY(Future)
> ```
> would waiting `Future` to complete or timeout after 15 seconds.

Should not it be "Naive"? The "Naïve" does not like to be a english word?


- Guangya


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


On 九月 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated 九月 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38141: Refactor shared paths in provisioners.

[Jiang Yan Xu]

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

Ship it!


LGTM!

Could you run `sudo make check GTEST_FILTER="*Provisioner*"` to make sure Appc 
tests pass?

I originally consolidated all path manipulations into a single paths.hpp|cpp 
but now with provisioner dir part pulled out it may make sense to put the store 
specific ones into the store itself, but that's for another patch I will take 
on myself.


src/slave/containerizer/provisioners/appc/paths.cpp (lines 21 - 23)


No longer needed.



src/slave/containerizer/provisioners/appc/paths.cpp (line 25)


Ditto.



src/slave/containerizer/provisioners/appc/paths.cpp (line 28)


Ditto.



src/slave/containerizer/provisioners/appc/paths.cpp (line 31)


Ditto.



src/slave/containerizer/provisioners/appc/paths.cpp (line 87)


A single blank line.



src/slave/containerizer/provisioners/paths.hpp (lines 46 - 48)


This comment was assuming this was for APPC. Now that it's generic, it 
should be something like:

```
// NOTE: Each container could have multiple image types, therefore there
// can be the same  directories under each provisioner e.g.,
// /provisioners/DOCKER, /provisioners/APPC, etc. 
```



src/slave/containerizer/provisioners/paths.hpp (lines 49 - 51)


There is a "For appc" here, can the same be true for docker? I think it 
could.


- Jiang Yan Xu


On Sept. 7, 2015, 2:31 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38141/
> ---
> 
> (Updated Sept. 7, 2015, 2:31 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactor shared paths in provisioners.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f574e7e08c4b1aebed0fac39140c19adfe 
>   src/slave/containerizer/provisioners/appc.cpp 
> fc5ee19df51a3543aaf01c2301b976700610ff57 
>   src/slave/containerizer/provisioners/appc/paths.hpp 
> fb3a1a7295809d745dd15bee6db1f7e8dd99ab33 
>   src/slave/containerizer/provisioners/appc/paths.cpp 
> e6be851e24886d7c886adad4c7ea29ded17bdcbe 
>   src/slave/containerizer/provisioners/paths.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/paths.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/38141/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

[Wojciech Sielski]

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

Ship it!


Ship It!

- Wojciech Sielski


On Sept. 7, 2015, 6:47 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated Sept. 7, 2015, 6:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38158]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 2:33 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38158/
> ---
> 
> (Updated Sept. 8, 2015, 2:33 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Till Toenshoff.
> 
> 
> Bugs: MESOS-3051
> https://issues.apache.org/jira/browse/MESOS-3051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The goal of this refactoring was to reuse the Ranges objects as much as 
> possible, as prior there was substantial time spend in allocation/destruction 
> (MESOS-3051).
> 
> 
> Diffs
> -
> 
>   include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
>   src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 
> 
> Diff: https://reviews.apache.org/r/38158/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Guangya Liu]

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



docs/mesos-testing-patterns.md (line 10)


What does "Naïve" means?


- Guangya Liu


On 九月 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated 九月 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38193: Remove comment from updateAllocation

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On Sept. 9, 2015, 12:02 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38193/
> ---
> 
> (Updated Sept. 9, 2015, 12:02 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The allocation for updateAllocation will always be updated after
> reserve, unreserve, create and destroy, so there is no need to
> add the TODO comments of "Do not log if there is no update".
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> cb4020dea897ef198cd9898cabecf61edfade834 
> 
> Diff: https://reviews.apache.org/r/38193/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 37427: Docker registry: adding TokenManager.

[Timothy Chen]

> On Sept. 2, 2015, 8:01 p.m., Anand Mazumdar wrote:
> > src/Makefile.am, line 761
> > 
> >
> > Extra space ?
> 
> Jojy Varghese wrote:
> looks to be aligned with the rest
> 
> Anand Mazumdar wrote:
> The '\''s at the end does not seem to be aligned.

I'll fx it


- Timothy


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


On Sept. 2, 2015, 10:50 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37427/
> ---
> 
> (Updated Sept. 2, 2015, 10:50 p.m.)
> 
> 
> Review request for mesos, Lily Chen, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changes:
>   - Added Token implementation (RFC 7519).
>   - Added TokenManager implementation. This component keeps a cache of tokens
>   requested for any future requests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7b4d9f65506e7fa8425966009401aae73cdb79a5 
>   src/slave/containerizer/provisioners/docker/token_manager.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/docker/token_manager.cpp PRE-CREATION 
>   src/tests/provisioners/docker_provisioner_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37427/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 38197: Increment a metric when master drop a message

[Guangya Liu]

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

Review request for mesos, Ben Mahler, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

Increment a metric when master drop a message


Diffs
-

  src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 

Diff: https://reviews.apache.org/r/38197/diff/


Testing
---


Thanks,

Guangya Liu

Review Request 38149: Add message_call_received metrics for master

[Guangya Liu]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Add message_call_received metrics for master


Diffs
-

  src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
  src/master/metrics.hpp 2d07a16f2dc6811973c82259c3cccff07401b542 
  src/master/metrics.cpp d79206f08181c99641bef697be57b5d57b627285 
  src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
  src/tests/metrics_tests.cpp 3e9d7c2eadfcdd2522d65e4a55dc76808ba39159 

Diff: https://reviews.apache.org/r/38149/diff/


Testing
---


Thanks,

Guangya Liu

Re: Review Request 38160: Documented how to expedite event firing.

[haosdent huang]

> On Sept. 8, 2015, 3:44 p.m., Guangya Liu wrote:
> > docs/mesos-testing-patterns.md, line 10
> > 
> >
> > What does "Naïve" means?
> 
> haosdent huang wrote:
> Naïve waiting means just waiting until timeout. Like:
> ```
> AWAIT_READY(Future)
> ```
> would waiting `Future` to complete or timeout after 15 seconds.
> 
> Guangya Liu wrote:
> Should not it be "Naive"? The "Naïve" does not like to be a english word?

https://en.wiktionary.org/wiki/na%C3%AFve


- haosdent


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


On Sept. 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Sept. 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 37703: Add docker exec command.

[haosdent huang]

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

(Updated Sept. 9, 2015, 2:27 a.m.)


Review request for mesos and Timothy Chen.


Changes
---

Update according @tnachen reviews.


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


Repository: mesos


Description
---

Add docker exec command.


Diffs (updated)
-

  src/docker/docker.hpp aaf8884a20901ad3a440d545d4b6316b87fc3403 
  src/docker/docker.cpp 553e831029454d6d423842915b8bbfcaf19fa7f9 
  src/tests/containerizer/docker_tests.cpp 
babc7d8da4ed9d13b14bd69decd7f27fc7dfde89 

Diff: https://reviews.apache.org/r/37703/diff/


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 37168: MESOS-3063 (Add an example framework using dynamic reservation)

[Klaus Ma]

Hi Michael,

Do you have more comments about this example?

On 2015年09月06日 21:28, Klaus Ma wrote:
This is an automatically generated e-mail. To reply, visit: 
https://reviews.apache.org/r/37168/



On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote:

src/Makefile.am


(Diff revision 4)


1463

Any reason for this blank line here?

It's not necessary, will delete it.


On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote:

src/examples/dynamic_reservation_framework.cpp


(Diff revision 4)


281 

 START,   // The framework get the offer for the first time.

s/get/receives

OK :).


On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote:

src/examples/dynamic_reservation_framework.cpp


(Diff revision 4)


283 

 TASK_DONE,   // All tasks are done.

s/TASK_DONE/TASKS_DONE


Please correct me if I am wrong: TASK_DONE is a global state
across all slaves, while the other states can differ per
state, or? Maybe just extend the comment a bit explaining this.

Yes, TASK_DONE is a global state; when all task finished, framework 
will un-reserve all resources.



On September 6th, 2015, 8:49 a.m. UTC, *Joerg Schad* wrote:

src/tests/dynamic_reservation_framework_test.sh


(Diff revision 4)


30  

export MESOS_ROLES="test"

Do we need this here and below in script.cpp?

Yes, it's necessary because: 1. dynamic reservation is role based 2. 
can not use default role for dynamic reservation, neither * nor 
MESOS_DEFAULT_ROLE



- Klaus


On September 6th, 2015, 4:11 a.m. UTC, Klaus Ma wrote:

Review request for mesos and Michael Park.
By Klaus Ma.

/Updated Sept. 6, 2015, 4:11 a.m./

*Bugs: * MESOS-3063 
*Repository: * mesos


  Description

Provide example for dynamic reservation features.


  Testing

make make check


  Diffs

  * src/Makefile.am (5fdca0f)
  * src/examples/dynamic_reservation_framework.cpp (PRE-CREATION)
  * src/tests/dynamic_reservation_framework_test.sh (PRE-CREATION)
  * src/tests/examples_tests.cpp (3f56b30)
  * src/tests/flags.hpp (06da36d)
  * src/tests/script.cpp (bcc1fab)

View Diff 



--
Klaus Ma (马达), PMP® | http://www.cguru.net

Re: Review Request 37284: Maintenance Primitives: Added support for Accept / Decline of InverseOffers in master.

[Yong Qiao Wang]

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

Ship it!


Ship It!

- Yong Qiao Wang


On Sept. 2, 2015, 7:33 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37284/
> ---
> 
> (Updated Sept. 2, 2015, 7:33 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
> 
> Diff: https://reviews.apache.org/r/37284/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37283: Maintenance Primitives: Refactored Master maintenance test to use V1 API.

[Yong Qiao Wang]

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

Ship it!


Ship It!

- Yong Qiao Wang


On Sept. 2, 2015, 7:32 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37283/
> ---
> 
> (Updated Sept. 2, 2015, 7:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/master_maintenance_tests.cpp 
> fb8dca3757a9565d5eb5a69eed10aa34602bb15c 
> 
> Diff: https://reviews.apache.org/r/37283/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 38201: [MESOS-1187] precision errors with allocation calculations

[Klaus Ma]

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

Review request for mesos.


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


Repository: mesos


Description
---

As allocations are stored/transmitted as doubles many a times precision errors 
creep in.


Diffs
-

  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 

Diff: https://reviews.apache.org/r/38201/diff/


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 38201: [MESOS-1187] precision errors with allocation calculations

[Guangya Liu]

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


I think that the V1 related should also be updated.

- Guangya Liu


On 九月 9, 2015, 3:15 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38201/
> ---
> 
> (Updated 九月 9, 2015, 3:15 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-1187
> https://issues.apache.org/jira/browse/MESOS-1187
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As allocations are stored/transmitted as doubles many a times precision 
> errors creep in.
> 
> 
> Diffs
> -
> 
>   src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
>   src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
> 
> Diff: https://reviews.apache.org/r/38201/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 38191: Removing unused Executor protobuf

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38191]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 10:30 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38191/
> ---
> 
> (Updated Sept. 8, 2015, 10:30 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Ben Mahler, and Vinod Kone.
> 
> 
> Bugs: MESOS-3393
> https://issues.apache.org/jira/browse/MESOS-3393
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The executor protobuf definition living outside the v1/ directory is unused, 
> it should be removed to avoid confusion.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor/executor.hpp 85f181c 
>   include/mesos/executor/executor.proto 52c84b3 
>   src/Makefile.am 5fdca0f 
> 
> Diff: https://reviews.apache.org/r/38191/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in libprocess.

[haosdent huang]

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

(Updated Sept. 9, 2015, 2:38 a.m.)


Review request for mesos, Artem Harutyunyan, Alex Clemmer, and Joseph Wu.


Changes
---

Update according @kaysoky reviews.


Repository: mesos


Description
---

Add CMake macro VsBuildCommand in libprocess.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/CMakeLists.txt 
997cc0d0e316e316136d4746e50e9e292a82b36b 
  3rdparty/libprocess/cmake/ProcessConfigure.cmake 
12506a1369de005285268f895f365aba0c560f78 
  3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 
  3rdparty/libprocess/cmake/macros/Noop.cmake PRE-CREATION 
  3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 
  3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION 

Diff: https://reviews.apache.org/r/37273/diff/


Testing
---


Thanks,

haosdent huang

Re: Review Request 37273: [1/2]Add CMake macro VsBuildCommand in libprocess.

[haosdent huang]

> On Sept. 8, 2015, 9:44 p.m., Joseph Wu wrote:
> > Just a bit more cleanup, and I think this would be good to go.
> > 
> > If possible, you should coordinate with Alex Clemmer to make sure this 
> > change doesn't conflict/break what he's working on 
> > (https://reviews.apache.org/r/37019/).

Thanks a lot.


- haosdent


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


On Sept. 9, 2015, 2:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37273/
> ---
> 
> (Updated Sept. 9, 2015, 2:38 a.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Alex Clemmer, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add CMake macro VsBuildCommand in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/CMakeLists.txt 
> 997cc0d0e316e316136d4746e50e9e292a82b36b 
>   3rdparty/libprocess/cmake/ProcessConfigure.cmake 
> 12506a1369de005285268f895f365aba0c560f78 
>   3rdparty/libprocess/cmake/ProcessTestsConfigure.cmake 
> 9e4dcb83a8cc4e95a2a38573944f6b38e2eac76e 
>   3rdparty/libprocess/cmake/macros/Noop.cmake PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.bat PRE-CREATION 
>   3rdparty/libprocess/cmake/macros/VsBuildCommand.cmake PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/37273/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 38173: Stout: Simplified hashmap implementation.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38171, 38172, 38173]

All tests passed.

- Mesos ReviewBot


On Sept. 8, 2015, 9:52 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38173/
> ---
> 
> (Updated Sept. 8, 2015, 9:52 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-3217
> https://issues.apache.org/jira/browse/MESOS-3217
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/hashmap.hpp 
> f09bea125035aa3621402b83608b233e42877559 
> 
> Diff: https://reviews.apache.org/r/38173/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 37234: Maintenance Primitives: Added URL field to InverseOffer proto.

[Yong Qiao Wang]

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

Ship it!


Ship It!

- Yong Qiao Wang


On Sept. 2, 2015, 7:32 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37234/
> ---
> 
> (Updated Sept. 2, 2015, 7:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joseph Wu.
> 
> 
> Bugs: MESOS-1474
> https://issues.apache.org/jira/browse/MESOS-1474
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 56bcbcc08fa0f98416c5048080adb25efc588019 
> 
> Diff: https://reviews.apache.org/r/37234/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[Bernd Mathiske]

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



docs/mesos-testing-patterns.md (line 16)


s/the registration/registration
s/registration request/a registration request



docs/mesos-testing-patterns.md (line 55)


s/re-/


- Bernd Mathiske


On Sept. 7, 2015, 9:04 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Sept. 7, 2015, 9:04 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38158: Refactored Value::Ranges coalesce().

[Joerg Schad]

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

(Updated Sept. 8, 2015, 2:33 p.m.)


Review request for mesos, Bernd Mathiske and Till Toenshoff.


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


Repository: mesos


Description
---

The goal of this refactoring was to reuse the Ranges objects as much as 
possible, as prior there was substantial time spend in allocation/destruction 
(MESOS-3051).


Diffs (updated)
-

  include/mesos/values.hpp e300580431f7fd6cff06e9617c0227b51c4cb8e2 
  src/common/values.cpp 750264e603b4cde2011f07f4434a4b34fe3e512f 
  src/tests/resources_tests.cpp 2ae93a9c8235e5e4643539d409df51c39c6d7e56 
  src/tests/values_tests.cpp fc35d97894a2de6207b9337180e2160e6f2cb1f5 

Diff: https://reviews.apache.org/r/38158/diff/


Testing
---

make check


Thanks,

Joerg Schad

Re: Review Request 38165: MESOS-3377 - Adding CONTAINER_NAME as additional env variable

[haosdent huang]

> On Sept. 8, 2015, 2:57 p.m., Wojciech Sielski wrote:
> > Ship It!

Hi, @sielaq Could you add @tnachen in reviewers? Because he is the maintainer 
of docker container component, I think he could help decide this approach 
should be accepted or not. And then help you commit this if this is accepted.


- haosdent


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


On Sept. 7, 2015, 6:47 p.m., Wojciech Sielski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38165/
> ---
> 
> (Updated Sept. 7, 2015, 6:47 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3377
> https://issues.apache.org/jira/browse/MESOS-3377
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3377 - Adding CONTAINER_NAME as additional env variable
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp ec2de5436ef5261a7d04eebeeded58435774cd9e 
> 
> Diff: https://reviews.apache.org/r/38165/diff/
> 
> 
> Testing
> ---
> 
> Every docker container that is created by mesos,
> gonna get conatiner dynamic env variable CONTAINER_NAME.
> So it is easy to use by 3rd party application,
> for example: when use with registartor/consul 
> so is it's much easier to deregister itself
> (or put into maintanance mode) before being killed.
> 
> 
> Tested and verified afetr build locally.
> 
> 
> Thanks,
> 
> Wojciech Sielski
> 
>

Re: Review Request 38160: Documented how to expedite event firing.

[haosdent huang]

> On Sept. 8, 2015, 3:44 p.m., Guangya Liu wrote:
> > docs/mesos-testing-patterns.md, line 10
> > 
> >
> > What does "Naïve" means?

Naïve waiting means just waiting until timeout. Like:
```
AWAIT_READY(Future)
```
would waiting `Future` to complete or timeout after 15 seconds.


- haosdent


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


On Sept. 7, 2015, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38160/
> ---
> 
> (Updated Sept. 7, 2015, 4:04 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/mesos-testing-patterns.md d0d92ff569012677043a03412462cf4384096ba9 
> 
> Diff: https://reviews.apache.org/r/38160/diff/
> 
> 
> Testing
> ---
> 
> doc rendered in Markdown
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 38137: Added Docker provisioner and local store

[Timothy Chen]

> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner.hpp, line 54
> > 
> >
> > doxygen style?

I'll fix this later with another patch, this isn't related to docker.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioner.cpp, line 36
> > 
> >
> > I think we should guard against multiple "create" calls. Call to 
> > "create" multiple times here would repeat the whole logic of creating the 
> > creator map and provsioners.

Ditto.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 67
> > 
> >
> > I believe the default value of an option is None.So need not explicitly 
> > declare.

It's provided so users can skip providing a value all together.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.hpp, line 94
> > 
> >
> > Is it layers or just layer ids? If its just layer ids, then the struct 
> > should be DockerImageInfo (and not DockerImage).

Not sure what the difference between DockerImageInfo and DockerImage is. But 
it's just layer ids.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 56
> > 
> >
> > I strongly believe that we should have strong/explicit ownership 
> > semantics for pointers (instead of raw pointers).

Sure, perhaps you can have a new patch that fixes from the interface to all the 
implementations.
I rather not do this in this patch.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 111
> > 
> >
> > should we create a guard here so that multiple calls to "create" wont 
> > call multiple "process" create?

We assume each create is a seperate new process.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 172
> > 
> >
> > should we check/validate for the docker_rootfs_dir?

This is removed.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker.cpp, line 279
> > 
> >
> > since destroy can be called from multiple thread contexts, should we 
> > proetct it?

It's libprocess, so no.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 94
> > 
> >
> > s/refStore/referenceStore?

Renamed to metadataManager.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 157
> > 
> >
> > here it is possible that refStore could be not created and we still 
> > have a LocalStoreProcess object. Maybe we should move the refStore creation 
> > to before line 152.

Thanks this is already fixed in latest revision.


> On Sept. 8, 2015, 6:35 p.m., Jojy Varghese wrote:
> > src/slave/containerizer/provisioners/docker/local_store.cpp, line 174
> > 
> >
> > Can this return an error?

This is already fixed in latest revision


- Timothy


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


On Sept. 8, 2015, 7:52 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38137/
> ---
> 
> (Updated Sept. 8, 2015, 7:52 p.m.)
> 
> 
> Review request for mesos, Jojy Varghese, Till Toenshoff, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Joining all the commits around provisioner and local store into one review so 
> it's easier to review, as patches
> are changing code on top of each other.
> 
> All the commits are going to committed together.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5fdca0f 
>   src/messages/docker_provisioner.hpp PRE-CREATION 
>   src/messages/docker_provisioner.proto PRE-CREATION 
>   

Re: Review Request 38149: Add message_call_received metrics for master

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [38149]

All tests passed.

- Mesos ReviewBot


On Sept. 9, 2015, 1:42 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38149/
> ---
> 
> (Updated Sept. 9, 2015, 1:42 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add message_call_received metrics for master
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 5589eca4317b597de509f3387cfc349083b361ac 
>   src/master/metrics.hpp 2d07a16f2dc6811973c82259c3cccff07401b542 
>   src/master/metrics.cpp d79206f08181c99641bef697be57b5d57b627285 
>   src/tests/master_tests.cpp 8a6b98b9f59ead20f537eb60b5084feed069a5b1 
>   src/tests/metrics_tests.cpp 3e9d7c2eadfcdd2522d65e4a55dc76808ba39159 
> 
> Diff: https://reviews.apache.org/r/38149/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>