Re: Review Request 35467: Fix a comment in Slave.

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On June 15, 2015, 5:37 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35467/
> ---
> 
> (Updated June 15, 2015, 5:37 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 
> 
> Diff: https://reviews.apache.org/r/35467/diff/
> 
> 
> Testing
> ---
> 
> Comment fix.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35433: CHECK that checkpointed resources exist on the slave.

[Jie Yu]

> On June 14, 2015, 10:46 a.m., Benjamin Hindman wrote:
> > Just so I understand, does this mean if we happen to get in the unfortunate 
> > situation where a slave has neglected to get the dynamic reservation 
> > because it was just starting up and then it gets the task launch it will 
> > shutdown the slave because the CHECK will fail? I would expect the slave to 
> > simply send a TASK_LOST. Said another way, this is not an assertion our 
> > code guarantees. If instead we were waiting for some kind of an ack from 
> > the slave that it received the dynamic reservation before it send the task 
> > launch then a CHECK would make sense.

We don't expect this to happen because we always send a 
CheckpointResourcesMessage before sending the task to the slave and TCP ensures 
in order delivery (out of order delivery is possible if two sockets are used. 
it's possible because the way we create ephemeral connections, but this is very 
unlikely to happen). Master won't send the task to the slave if the slave 
hasn't registered.

I would rather keep the CHECK here unless we found that this is a real issue 
(and then we can change that to send status update).


- Jie


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


On June 15, 2015, 12:39 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35433/
> ---
> 
> (Updated June 15, 2015, 12:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> No bug was observed (yet), but realized I forgot about this in the dynamic 
> reservations patches.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 
> 
> Diff: https://reviews.apache.org/r/35433/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 34427: AppC provisioner backend using bind mounts.

[Till Toenshoff]

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



src/slave/containerizer/provisioners/appc/bind_backend.hpp


Given that you dont declare any templates, would it make sense to split 
this into cpp+hpp?



src/slave/containerizer/provisioners/appc/bind_backend.hpp





- Till Toenshoff


On May 19, 2015, 6:46 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34427/
> ---
> 
> (Updated May 19, 2015, 6:46 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note: This is a specialized backend; see notes in bind_backend.hpp.
>  
> Reproduced here for your convenience:
> 
> This is a specialized backend that may be useful for deployments
> using large (multi-GB) single-layer images *and* where more
> recent kernel features such as overlayfs are not available. For small
> images (10's to 100's of MB) the Copy backend may be sufficient.
> 1) It supports only a single layer. Multi-layer images will fail
>to provision and the container will fail to launch!
> 2) The filesystem is read-only because all containers using this
>image share the source. Select writable areas can be achieved
>by
>mounting read-write volumes to places like /tmp, /var/tmp,
>/home, etc. using the ContainerInfo. These can be relative to
>the executor work directory.
> 3) It relies on the image persisting in the store.
> 4) It's fast because the bind mount requires (nearly) zero IO.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 34755cf795391c9b8051a5e4acc6caf844984496 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/bind_backend.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34427/diff/
> 
> 
> Testing
> ---
> 
> manual testing of a single layer image with RW relative bind mount for /tmp.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34908: Rename --docker_sandbox_directory flag for general use.

[Till Toenshoff]

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

Ship it!



docs/configuration.md


Do you have a followup planned that also updates "docs/upgrades.md" on this 
configuration change?


- Till Toenshoff


On June 1, 2015, 9:48 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34908/
> ---
> 
> (Updated June 1, 2015, 9:48 p.m.)
> 
> 
> Review request for mesos and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Rename --docker_sandbox_directory flag for general use.
> 
> This will require users to update configuration scripts etc if they specify 
> the old flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 4e20913cf7a088997235f399733c89db3441fb7c 
>   src/slave/containerizer/docker.cpp 41e0b98c387e57b692df4c56ae21ce70f67f9a19 
>   src/slave/flags.hpp 84dbb8a2aa963bb38cf0f610ab442b626179b173 
>   src/slave/flags.cpp ab8709861eb037e75dd2e91a097048ea6cd20c9c 
>   src/tests/docker_containerizer_tests.cpp 
> 3a983c6813dab6fa03ccb2c87e1ea71866766d6e 
> 
> Diff: https://reviews.apache.org/r/34908/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 34943: Added validation to flags.

[Till Toenshoff]

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


Long missed feature indeed :) ... thanks for reviving this.

You might want to remove the dependency from the "blocked" but discarded RR to 
allow the buildbot to trigger.


3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


Guess you also want to capture `validate`.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


See above.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


See above.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


See above.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


s/loader/load/g


- Till Toenshoff


On June 15, 2015, 5:52 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> ---
> 
> (Updated June 15, 2015, 5:52 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, 
> enabling us to get rid of our "loader" and "stringifier" functors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
> 87606d860dea3235ec70d127d3379065d42dc90b 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 
> 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp 
> fda5ae1328a23a4371475652f921341dce7448d5 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 80450185f60c5b273face490e0bb9e695b0cb984 
> 
> Diff: https://reviews.apache.org/r/34943/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 35482: Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [35482]

Failed command: ./support/apply-review.sh -n -r 35482

Error:
 2015-06-16 00:15:18 URL:https://reviews.apache.org/r/35482/diff/raw/ 
[44012/44012] -> "35482.patch" [1]
error: patch failed: src/Makefile.am:353
error: src/Makefile.am: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On June 16, 2015, 12:14 a.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35482/
> ---
> 
> (Updated June 16, 2015, 12:14 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replace adhoc JSON conversion functions for ResourceStatistics with a 
> protocol buffer to JSON converter.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 3c44f01f7d22cff6b9e46c2e68997ce5c2773791 
>   src/common/json.hpp PRE-CREATION 
>   src/common/json.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/network/port_mapping.hpp 
> 4c719b186b519fad0c3869dbdae8b60c3a2c20cc 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 432b05ce5a99c8239fafc47a6b65d46a0fbac26e 
>   src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 
> 
> Diff: https://reviews.apache.org/r/35482/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Review Request 35482: Replace adhoc JSON conversion functions for ResourceStatistics with a protocol buffer to JSON converter.

[Paul Brett]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Replace adhoc JSON conversion functions for ResourceStatistics with a protocol 
buffer to JSON converter.


Diffs
-

  src/Makefile.am 3c44f01f7d22cff6b9e46c2e68997ce5c2773791 
  src/common/json.hpp PRE-CREATION 
  src/common/json.cpp PRE-CREATION 
  src/slave/containerizer/isolators/network/port_mapping.hpp 
4c719b186b519fad0c3869dbdae8b60c3a2c20cc 
  src/slave/containerizer/isolators/network/port_mapping.cpp 
432b05ce5a99c8239fafc47a6b65d46a0fbac26e 
  src/tests/port_mapping_tests.cpp f8372df74cd71df37de4a2438069ef0ea8878512 

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


Testing
---

sudo make check


Thanks,

Paul Brett

Re: Review Request 35129: Refactor Future::Data to use Result. Remove dynamic allocation.

[Michael Park]

> On June 15, 2015, 11:47 p.m., Michael Park wrote:
> > Ship It!
> 
> Michael Park wrote:
> Looks like you might have to rebase this one more time?

Never mind, it applies just fine.


- Michael


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


On June 14, 2015, 4:17 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35129/
> ---
> 
> (Updated June 14, 2015, 4:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Bugs: MESOS-2801
> https://issues.apache.org/jira/browse/MESOS-2801
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> adfad6faf89a52bf2da90d10a29e3d34502898bd 
> 
> Diff: https://reviews.apache.org/r/35129/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35129: Refactor Future::Data to use Result. Remove dynamic allocation.

[Michael Park]

> On June 15, 2015, 11:47 p.m., Michael Park wrote:
> > Ship It!

Looks like you might have to rebase this one more time?


- Michael


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


On June 14, 2015, 4:17 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35129/
> ---
> 
> (Updated June 14, 2015, 4:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Bugs: MESOS-2801
> https://issues.apache.org/jira/browse/MESOS-2801
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> adfad6faf89a52bf2da90d10a29e3d34502898bd 
> 
> Diff: https://reviews.apache.org/r/35129/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 34943: Added validation to flags.

[Niklas Nielsen]

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


Very neat!

LGTM - however, my gut feeling is that we should only include the signature 
comment once and reference it from the other call sites.
Other than that, only a question about whether we should do CHECK_NOTNULL on 
the pointers you take in.


3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


Should we have a CHECK_NOTNULL for this option?



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


I am a bit torn whether we should copy this block so many places (taken the 
number of times it needs to get updates/kept in sync).

Should we reference the first block from the other places?


- Niklas Nielsen


On June 15, 2015, 10:52 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> ---
> 
> (Updated June 15, 2015, 10:52 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, 
> enabling us to get rid of our "loader" and "stringifier" functors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
> 87606d860dea3235ec70d127d3379065d42dc90b 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 
> 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp 
> fda5ae1328a23a4371475652f921341dce7448d5 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 80450185f60c5b273face490e0bb9e695b0cb984 
> 
> Diff: https://reviews.apache.org/r/34943/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 35129: Refactor Future::Data to use Result. Remove dynamic allocation.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On June 14, 2015, 4:17 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35129/
> ---
> 
> (Updated June 14, 2015, 4:17 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Bugs: MESOS-2801
> https://issues.apache.org/jira/browse/MESOS-2801
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> adfad6faf89a52bf2da90d10a29e3d34502898bd 
> 
> Diff: https://reviews.apache.org/r/35129/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35179]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 11:23 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> ---
> 
> (Updated June 15, 2015, 11:23 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
> https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> ---
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Anand Mazumdar]

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

(Updated June 15, 2015, 11:23 p.m.)


Review request for mesos, Adam B and Cody Maloney.


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


Repository: mesos


Description
---

This change takes an un-complicated/naive route ( no trimming of values etc ) 
at making path::join(...) variadic mainly in order to preserve the earlier 
over-loaded join functionality.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
cf354125687e0f60b6d5b105f19d75e4436f21bf 

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


Testing
---

make check + added some additional tests.


Thanks,

Anand Mazumdar

Re: Review Request 35441: Removed unused macros.

[Alexander Rojas]

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

Ship it!


Ship It!

- Alexander Rojas


On June 14, 2015, 7:26 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35441/
> ---
> 
> (Updated June 14, 2015, 7:26 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 95b4b33b70c37640d97dff5d5724550d42048b76 
> 
> Diff: https://reviews.apache.org/r/35441/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 30339: Call hookmanager only if some hooks are installed.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [30339]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 10:26 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> ---
> 
> (Updated June 15, 2015, 10:26 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Call hook manager only if hooks were specified on the commandline.
> 
> 
> Diffs
> -
> 
>   src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d 
>   src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b 
>   src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353 
>   src/slave/containerizer/containerizer.cpp 
> e995ce602261c18373ac09c823638c4a252cca86 
>   src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 
> 
> Diff: https://reviews.apache.org/r/30339/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Michael Park]

> On June 15, 2015, 10:57 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, lines 49-51
> > 
> >
> > Not yours, but could you fix the formatting here to not wrap after 
> > `return` please?
> > 
> > ```
> >   return strings::remove(path1, "/", strings::SUFFIX) + "/" +
> >  strings::remove(path2, "/", strings::PREFIX);
> > ```

Not sure why this got duplicated.


- Michael


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


On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> ---
> 
> (Updated June 15, 2015, 10:45 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
> https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> ---
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Michael Park]

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



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Maybe `s/Default/Base/`?



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Not yours, but could you fix the formatting here to not wrap after `return` 
please?

```
  return strings::remove(path1, "/", strings::SUFFIX) + "/" +
 strings::remove(path2, "/", strings::PREFIX);
```



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Not yours, but could you fix the formatting here to not wrap after `return` 
please?

```
  return strings::remove(path1, "/", strings::SUFFIX) + "/" +
 strings::remove(path2, "/", strings::PREFIX);
```



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


`s/path1 ,/path1,`


- Michael Park


On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> ---
> 
> (Updated June 15, 2015, 10:45 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
> https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> ---
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Anand Mazumdar]

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

(Updated June 15, 2015, 10:45 p.m.)


Review request for mesos, Adam B and Cody Maloney.


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


Repository: mesos


Description
---

This change takes an un-complicated/naive route ( no trimming of values etc ) 
at making path::join(...) variadic mainly in order to preserve the earlier 
over-loaded join functionality.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
cf354125687e0f60b6d5b105f19d75e4436f21bf 

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


Testing
---

make check + added some additional tests.


Thanks,

Anand Mazumdar

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Anand Mazumdar]

> On June 15, 2015, 9:50 p.m., Cody Maloney wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp, line 50
> > 
> >
> > Looks like there is an accidental space addded after path1 here

My bad, Fixed.


- Anand


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


On June 15, 2015, 10:45 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> ---
> 
> (Updated June 15, 2015, 10:45 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
> https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> ---
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 35403: Added the ability to get the peer address of a connected or accepted Socket.

[Ben Mahler]

> On June 12, 2015, 11:06 p.m., Niklas Nielsen wrote:
> > Maybe add @joris to this review?

This change is pretty trivial, it's symmetric to the existing Socket::address, 
but I'd be happy to follow up if he has any feedback! :)


- Ben


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


On June 12, 2015, 10:40 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35403/
> ---
> 
> (Updated June 12, 2015, 10:40 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See above.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 6d949c0b9b5bfd2b54be76a08ab2b6970be5d4fa 
>   3rdparty/libprocess/include/process/socket.hpp 
> b8c2274de535ac473e49a09165b601c96d3ebe8b 
>   3rdparty/libprocess/src/socket.cpp 0e1cebb19e21c706b152d35a0b8722924c971a35 
> 
> Diff: https://reviews.apache.org/r/35403/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35404: Fixed http::Request::client to be set correctly.

[Ben Mahler]

> On June 12, 2015, 11:20 p.m., Jie Yu wrote:
> > Have you tested this?

Yep, the test was updated to properly test this, and I verified manually. :)


> On June 12, 2015, 11:20 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/src/process.cpp, line 593
> > 
> >
> > Not sure if this will cause a scalability issue since we are calling 
> > getpeername everytime there are some requests coming in. Maybe we should 
> > cache the peer address when the socket is accepted. Can you add a TODO?

Added a TODO on Socket::peer which matches the existing one in Socket::address. 
FWIW I measure about 1 microsecond for this call, excluding timer overhead, so 
should be ok without the caching, but I'd love to avoid the syscall overhead 
here! :)


- Ben


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


On June 12, 2015, 10:40 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35404/
> ---
> 
> (Updated June 12, 2015, 10:40 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2519
> https://issues.apache.org/jira/browse/MESOS-2519
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This now uses `Socket::peer` to augment the `http::Request`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> aadd7bb0ae12b93336900c76d8d5aaa4421ea198 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 8444b9c961d04b932188b2ac37e2a42aafda1abd 
> 
> Diff: https://reviews.apache.org/r/35404/diff/
> 
> 
> Testing
> ---
> 
> Updated the test.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 30339: Call hookmanager only if some hooks are installed.

[Kapil Arya]

> On June 4, 2015, 3:39 a.m., Till Toenshoff wrote:
> > src/tests/hook_tests.cpp, line 132
> > 
> >
> > Could you please explain quickly on why this was ever working / is 
> > needed now?

Good point. As it turns out, we don't need these changes anymore since we 
explicitly do `HookManager::initialize(HOOK_MODULE_NAME)`. This forces the 
hooksAvailable() to always return true. Thanks for the catch!


- Kapil


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


On June 15, 2015, 6:26 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> ---
> 
> (Updated June 15, 2015, 6:26 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Call hook manager only if hooks were specified on the commandline.
> 
> 
> Diffs
> -
> 
>   src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d 
>   src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b 
>   src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353 
>   src/slave/containerizer/containerizer.cpp 
> e995ce602261c18373ac09c823638c4a252cca86 
>   src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 
> 
> Diff: https://reviews.apache.org/r/30339/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 29406: Introduce libevent ssl socket.

[Michael Park]

> On June 15, 2015, 5:28 p.m., Jojy Varghese wrote:
> > 3rdparty/libprocess/include/process/address.hpp, line 84
> > 
> >
> > There are a few "if family == INET" (or similar) in the code. By 
> > specializing the net address structures on FAMILY, we will get rid of them. 
> > As most of the network structures (socket for example)  are classified 
> > based on family, this should naturally fit into the overall scheme.

I think we'll probably want a version-agnostic one as well as specialized ones 
if we care about supporting the differences IPv4 and IPv6. `Boost.Asio` and the 
proposed networking library for the C++ standard based on it provide `address`, 
`address_v4` and `address_v6` for example. We could provide the same 
alternatives via template specializations, but my point is that we'll probably 
want to keep the version-agnostic one for cases where we can't know or don't 
care which version we have.


- Michael


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


On June 13, 2015, 9:23 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29406/
> ---
> 
> (Updated June 13, 2015, 9:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Bugs: MESOS-1913
> https://issues.apache.org/jira/browse/MESOS-1913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Requires:
> configure --enable-libevent --enable-libevent-socket --enable-ssl
> New environment variables:
> ```
> SSL_ENABLED=(false|0,true|1)
> SSL_CERT_FILE=(path to certificate)
> SSL_KEY_FILE=(path to key)
> SSL_VERIFY_CERT=(false|0,true|1)
> SSL_REQUIRE_CERT=(false|0,true|1)
> SSL_VERIFY_DEPTH=(4)
> SSL_CA_DIR=(path to CA directory)
> SSL_CA_FILE=(path to CA file)
> SSL_CIPHERS=(accepted ciphers separated by ':')
> SSL_ENABLE_SSL_V2=(false|0,true|1)
> SSL_ENABLE_SSL_V3=(false|0,true|1)
> SSL_ENABLE_TLS_V1_0=(false|0,true|1)
> SSL_ENABLE_TLS_V1_1=(false|0,true|1)
> SSL_ENABLE_TLS_V1_2=(false|0,true|1)
> ```
> 
> Only TLSV1.2 is enabled by default.
> Use the `ENABLE_SSL_V*` and `ENABLE_TLS_V*` environment variables to open up 
> more protocols.
> Use the `SSL_CIPHERS` environment variable to restrict or open up the 
> supported ciphers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 489ce359f383d819299335cbaa8c95724b0c6ac2 
>   3rdparty/libprocess/include/process/address.hpp 
> 729f5cd7ea981e43a33c1fe9d99d58b906a31158 
>   3rdparty/libprocess/include/process/socket.hpp 
> b8c2274de535ac473e49a09165b601c96d3ebe8b 
>   3rdparty/libprocess/src/libevent.hpp 
> f6cc72178613a30446629532a773afccfd404212 
>   3rdparty/libprocess/src/libevent.cpp 
> fb038597358135a06c1927d079cb7cb09fea7452 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp PRE-CREATION 
>   3rdparty/libprocess/src/openssl.hpp PRE-CREATION 
>   3rdparty/libprocess/src/openssl.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> aadd7bb0ae12b93336900c76d8d5aaa4421ea198 
>   3rdparty/libprocess/src/socket.cpp 0e1cebb19e21c706b152d35a0b8722924c971a35 
> 
> Diff: https://reviews.apache.org/r/29406/diff/
> 
> 
> Testing
> ---
> 
> make check (uses non-ssl socket)
> benchmarks using ssl sockets
> master, slave, framework, webui launch with ssl sockets
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 30339: Call hookmanager only if some hooks are installed.

[Kapil Arya]

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

(Updated June 15, 2015, 6:26 p.m.)


Review request for mesos, Niklas Nielsen and Till Toenshoff.


Changes
---

Removed unneeded changes from hook tests.


Repository: mesos


Description
---

Call hook manager only if hooks were specified on the commandline.


Diffs (updated)
-

  src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d 
  src/hook/manager.cpp b43b91878cc9f0f5f5d081b9b3af0768c07c282b 
  src/master/master.cpp 0137c37a42d4f7a9057568a8cd7b662ed92be353 
  src/slave/containerizer/containerizer.cpp 
e995ce602261c18373ac09c823638c4a252cca86 
  src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 

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


Testing
---

make check


Thanks,

Kapil Arya

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

[Ben Mahler]

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

Ship it!


I personally find it easier to read if we don't have the special 'return' cases:

```
{
  if (!resources.empty()) {
// Do work.
  }
}

Seems clearer than:
{
  if (resources.empty()) {
// Don't do work by returning.
  }
  
  // Do work.
}
```

I think most of the cases where we've done the early returns were motivated by 
a few reasons that don't hold here:

(1) We wanted to avoid deep nesting of logic (using 'returns' allowed us to 
flatten the logical structure), and/or
(2) We wanted to eliminate many special cases in sequence, until only the 
correct case remains (e.g. message handlers, (a) can't lookup framework, (b) 
message sent by wrong pid, (c) invalid offers used, ...).

Up to you!


src/tests/examples_tests.cpp


Don't forget to delete this


- Ben Mahler


On June 15, 2015, 10:03 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 10:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
>   src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35473]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 10:03 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 10:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
>   src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

[Jie Yu]

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

(Updated June 15, 2015, 10:03 p.m.)


Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.


Changes
---

Review comments.


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


Repository: mesos


Description
---

Removed a few incorrect CHECKs in DRF sorter.

See details in the ticket for the reason why this is needed.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.cpp 
ac05afdc7d408735dd796faa68c943e75540aaa7 
  src/tests/examples_tests.cpp 41a9265e91ea54b657afbd32c22e204c5f64ca17 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Cody Maloney]

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



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Looks like there is an accidental space addded after path1 here


- Cody Maloney


On June 15, 2015, 3:25 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> ---
> 
> (Updated June 15, 2015, 3:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
> https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> ---
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

[Ben Mahler]

> On June 15, 2015, 9:40 p.m., Ben Mahler wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 176-188
> > 
> >
> > How about the same logic in both?
> > 
> > ```
> > {
> >   if (_resources.empty()) {
> > // Do work.
> >   }
> > }
> > ```
> > 
> > In the case of remove(), we can have a CHECK that the slave id is 
> > contained in the map as part of the work?

Sorry:

```
{
  if (!_resources.empty()) {
// Do work.
  }
}
```


- Ben


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


On June 15, 2015, 9:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 9:15 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

[Ben Mahler]

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



src/master/allocator/sorter/drf/sorter.cpp


How about the same logic in both?

```
{
  if (_resources.empty()) {
// Do work.
  }
}
```

In the case of remove(), we can have a CHECK that the slave id is contained 
in the map as part of the work?


- Ben Mahler


On June 15, 2015, 9:15 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35473/
> ---
> 
> (Updated June 15, 2015, 9:15 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2627
> https://issues.apache.org/jira/browse/MESOS-2627
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed a few incorrect CHECKs in DRF sorter.
> 
> See details in the ticket for the reason why this is needed.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> ac05afdc7d408735dd796faa68c943e75540aaa7 
> 
> Diff: https://reviews.apache.org/r/35473/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35411: Send oversubscribable resources during (re-)registration.

[Ben Mahler]

> On June 15, 2015, 8:21 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 498
> > 
> >
> > Which kind of update message? :)
> > s/update/'SlaveUpdate'/ ?

Changed it to "forwards the estimation" to match the other test comments, 
thanks!


> On June 15, 2015, 8:21 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 528
> > 
> >
> > You have the PIDs of the master and slave - would it make sense to be 
> > explicit in the pattern matching?

Since none of the tests in this file use the PID matching, I followed suit for 
consistency. It would be nice to use them in general, but there are cases where 
we don't have both PIDs yet (e.g. need to set up the expectation before 
starting the slave).


> On June 15, 2015, 8:21 p.m., Niklas Nielsen wrote:
> > src/tests/oversubscription_tests.cpp, line 538
> > 
> >
> > Should we const 'resources'?

Hm.. seems inconsistent with the rest of this file, but also, do we want to 
proliferate 'const' everywhere? There are a ton of scope variables in this file 
that can be const but are not, so I'm not concerned about it.

I haven't really noticed a lot of benefit from having scope variables as 
'const' where not necessary, have you? Would love to see some examples as the 
code is getting a bit inconsistent with 'const' usage. We have a ton of 
variables that can be const, but adding const to all of these might be too 
verbose IMO :(


- Ben


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


On June 15, 2015, 5:59 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35411/
> ---
> 
> (Updated June 15, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2866
> https://issues.apache.org/jira/browse/MESOS-2866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Send oversubscribable resources during (re-)registration.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 
>   src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 
>   src/tests/oversubscription_tests.cpp 
> e8ae053dd9cd712e49bd2830e414b7a3d99c20ca 
> 
> Diff: https://reviews.apache.org/r/35411/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 30339: Call hookmanager only if some hooks are installed.

[Kapil Arya]

> On June 3, 2015, 1:17 p.m., Niklas Nielsen wrote:
> > src/hook/manager.cpp, line 95
> > 
> >
> > Don't you need to acquire the mutex here?

Good catch. Fixed.


- Kapil


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


On June 15, 2015, 5:22 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30339/
> ---
> 
> (Updated June 15, 2015, 5:22 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Call hook manager only if hooks were specified on the commandline.
> 
> 
> Diffs
> -
> 
>   src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d 
>   src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955 
>   src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d 
>   src/slave/containerizer/containerizer.cpp 
> 4d66e767de1f877cb66b37826ba7c9d00639a7c0 
>   src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05 
>   src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1 
> 
> Diff: https://reviews.apache.org/r/30339/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 30339: Call hookmanager only if some hooks are installed.

[Kapil Arya]

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

(Updated June 15, 2015, 5:22 p.m.)


Review request for mesos, Niklas Nielsen and Till Toenshoff.


Changes
---

Added mutex check


Repository: mesos


Description
---

Call hook manager only if hooks were specified on the commandline.


Diffs (updated)
-

  src/hook/manager.hpp 638e19f5062f070cd0ec94d830615a2c73fd357d 
  src/hook/manager.cpp 54b0d34b6e9f2f8a8cf7a6c2f5ded2f6ab6c6955 
  src/master/master.cpp 710b8149c9d855d0f47cb2952366be10bc78c74d 
  src/slave/containerizer/containerizer.cpp 
4d66e767de1f877cb66b37826ba7c9d00639a7c0 
  src/slave/slave.cpp 271cb03770cd08406054dfce35d0821475e49b05 
  src/tests/hook_tests.cpp 3ffde6d6b2faeb5a8a40eb27c3b0a2b7f9ecd2b1 

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


Testing
---

make check


Thanks,

Kapil Arya

Review Request 35473: Removed a few incorrect CHECKs in DRF sorter.

[Jie Yu]

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

Review request for mesos, Ben Mahler, Niklas Nielsen, and Vinod Kone.


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


Repository: mesos


Description
---

Removed a few incorrect CHECKs in DRF sorter.

See details in the ticket for the reason why this is needed.


Diffs
-

  src/master/allocator/sorter/drf/sorter.cpp 
ac05afdc7d408735dd796faa68c943e75540aaa7 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 35441: Removed unused macros.

[Michael Park]

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

Ship it!


Ship It!

- Michael Park


On June 14, 2015, 5:26 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35441/
> ---
> 
> (Updated June 14, 2015, 5:26 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 95b4b33b70c37640d97dff5d5724550d42048b76 
> 
> Diff: https://reviews.apache.org/r/35441/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 35411: Send oversubscribable resources during (re-)registration.

[Niklas Nielsen]

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


Just wanted to tag on this review and follow the update changes - LGTM :)


src/tests/oversubscription_tests.cpp


Which kind of update message? :)
s/update/'SlaveUpdate'/ ?



src/tests/oversubscription_tests.cpp


You have the PIDs of the master and slave - would it make sense to be 
explicit in the pattern matching?



src/tests/oversubscription_tests.cpp


Should we const 'resources'?


- Niklas Nielsen


On June 15, 2015, 10:59 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35411/
> ---
> 
> (Updated June 15, 2015, 10:59 a.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2866
> https://issues.apache.org/jira/browse/MESOS-2866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Send oversubscribable resources during (re-)registration.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 
>   src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 
>   src/tests/oversubscription_tests.cpp 
> e8ae053dd9cd712e49bd2830e414b7a3d99c20ca 
> 
> Diff: https://reviews.apache.org/r/35411/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35441: Removed unused macros.

[Niklas Nielsen]

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

Ship it!


Ship It!

- Niklas Nielsen


On June 14, 2015, 10:26 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35441/
> ---
> 
> (Updated June 14, 2015, 10:26 a.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 95b4b33b70c37640d97dff5d5724550d42048b76 
> 
> Diff: https://reviews.apache.org/r/35441/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 35470: Fixed the flaky oversbuscription tests.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35470]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 7:02 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35470/
> ---
> 
> (Updated June 15, 2015, 7:02 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-2869
> https://issues.apache.org/jira/browse/MESOS-2869
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the flaky oversbuscription tests.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> e7d94cecb4a668f634e94fb0aba95155dc827510 
> 
> Diff: https://reviews.apache.org/r/35470/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> repeat the oversubscription tests for 100 times
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 35411: Send oversubscribable resources during (re-)registration.

[Vinod Kone]

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

Ship it!



src/slave/slave.cpp


s/oversubscribable/oversubscribed/



src/slave/slave.cpp


s/oversubscribable/oversubscribed/


- Vinod Kone


On June 15, 2015, 5:59 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35411/
> ---
> 
> (Updated June 15, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Niklas Nielsen, and Vinod Kone.
> 
> 
> Bugs: MESOS-2866
> https://issues.apache.org/jira/browse/MESOS-2866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Send oversubscribable resources during (re-)registration.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 
>   src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 
>   src/tests/oversubscription_tests.cpp 
> e8ae053dd9cd712e49bd2830e414b7a3d99c20ca 
> 
> Diff: https://reviews.apache.org/r/35411/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35470: Fixed the flaky oversbuscription tests.

[Ben Mahler]

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

Ship it!


Ship It!

- Ben Mahler


On June 15, 2015, 7:02 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35470/
> ---
> 
> (Updated June 15, 2015, 7:02 p.m.)
> 
> 
> Review request for mesos, Niklas Nielsen and Vinod Kone.
> 
> 
> Bugs: MESOS-2869
> https://issues.apache.org/jira/browse/MESOS-2869
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the flaky oversbuscription tests.
> 
> 
> Diffs
> -
> 
>   src/tests/oversubscription_tests.cpp 
> e7d94cecb4a668f634e94fb0aba95155dc827510 
> 
> Diff: https://reviews.apache.org/r/35470/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> repeat the oversubscription tests for 100 times
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 34361: converted hard-coded strings to consts

[Colin Williams]

> On June 10, 2015, 1:25 a.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, lines 3031-3034
> > 
> >
> > Why bother with all this? Why not just have `"key1"`, `"value1"`, 
> > `"key2"`, `"value2"` inlined appropriately throughout these tests. Very 
> > straightforward to read.
> 
> Colin Williams wrote:
> I think the issue with the changes remaining is that the test depends on 
> the same value occurring in several places. By consolidating to a variable 
> it's no longer possible for these values to get out of sync.
> 
> Niklas Nielsen wrote:
> Colin: exactly
> 
> Ben: We have label tests three places; in the master, slave and in the 
> modules and they use the same "foo", "bar", "baz", "qux" key value pairs.
> The idea was to centralize them, so they don't get out of date and to 
> avoid code duplication.
> 
> Does that make sense?
> 
> Ben Mahler wrote:
> Well, then let's just keep it simple and get rid of these special strings 
> by just making the tests use "key1", "value1", "key2", "value2", etc.
> 
> I'm not following your code duplication point, this introduces more code 
> (now there are additional global constants, which we like to avoid if 
> unnecessary).
> 
> Also, each test is ideally independent, why does the master's test for 
> labels affect the slave's test for labels? Let's say I need a test with 5 
> labels, you want to have 5x2=10 global constants to address this?
> 
> Niklas Nielsen wrote:
> Try to see how testLabelKey and testLabelValue was used previously and 
> "foo", "bar", "qux" was used on others and I created this ticket to unify the 
> way we do this, along with seeing these key value pair being created in the 
> slave and master tests.
> 
> Dispite the current patch, I do think we can simply and unify the test 
> label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
> which key pairs are being tested. The comments in the test code need to carry 
> a bunch of context, to make sense out of the label transformations 
> (especially across the library border for the hooks tests).
> 
> This is all in spirit of reducing the tech debt we introduced. We are on 
> the same page on not introducing more lines/key pairs than before. Let us 
> iterate on this and get back to you.
> 
> Colin Williams wrote:
> Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
> point of unifying label names into a centralized variable that aren't at all 
> related.
> 
> Niklas: I was a little confused by the original task description, so I 
> thought a sample patch would be a good discussion point. I don't entirely 
> understand the tech debt you're trying to solve. For example, it seems like 
> you're asking to have some constants defined somewhere that are used by both 
> master_test and slave_test, but the the similarities between these two only 
> seem incidental. I'm concerned that creating something like 
> CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
> 
> Niklas Nielsen wrote:
> Okay - thinking about this; I am on board with "key1", "value1" etc.
> 
> Colin: the tech debt is that we have some inlined constants (like "foo", 
> "bar", etc) and some which are constants (which have to be kept in sync 
> between hooks modules and test body).
> The idea was to unify the way we name the key value pairs.
> 
> Do you want to update this ticket to reflect this, or come with a new 
> patch set which tackles streaming the two?
> 
> Niklas Nielsen wrote:
> Ping :)

Sorry, my day job's been really all-consuming the last few days. I was planning 
to update the ticket.


- Colin


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


On June 8, 2015, 7:05 p.m., Colin Williams wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> ---
> 
> (Updated June 8, 2015, 7:05 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2637
> https://issues.apache.org/jira/browse/MESOS-2637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> converted hard-coded strings to consts
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 3ffde6d 
>   src/tests/master_tests.cpp ba3858f 
>   src/tests/slave_tests.cpp acae497 
> 
> Diff: https://reviews.apache.org/r/34361/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colin Williams
> 
>

Re: Review Request 33208: Delete detector in MesosSchedulerDriver::stop

[Niklas Nielsen]

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


Hey Robert; BenH helped out and wrote a PoC patch here 
https://reviews.apache.org/r/35405

In short; it is not safe to delete the detector at this point. The patch above 
does it in join() and has a good descriptive block of comment explaining the 
issue.
What we need to do is try it out and test it.

- Niklas Nielsen


On April 14, 2015, 10:23 p.m., Robert Lacroix wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33208/
> ---
> 
> (Updated April 14, 2015, 10:23 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Niklas Nielsen, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-1634
> https://issues.apache.org/jira/browse/MESOS-1634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the Mesos Scheduler Driver stops, it does not delete the detector 
> process before the object is garbage collected, which leaves ZooKeeper 
> connections hanging around unnecessarily. This deletes the process on stop as 
> well, not only on destruction.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 66fd2b3146568900356cc48e0f17c6720665ae80 
> 
> Diff: https://reviews.apache.org/r/33208/diff/
> 
> 
> Testing
> ---
> 
> make check, manual testing
> 
> 
> Thanks,
> 
> Robert Lacroix
> 
>

Re: Review Request 35410: Minor cleanups to the slave.

[Vinod Kone]

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

Ship it!


Ship It!

- Vinod Kone


On June 13, 2015, 2:04 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35410/
> ---
> 
> (Updated June 13, 2015, 2:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Minor cleanups to the slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 
> 
> Diff: https://reviews.apache.org/r/35410/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 34835: Add constexpr to C++ whitelist

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34835]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 5:59 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34835/
> ---
> 
> (Updated June 15, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Dave Lester, Ian Downes, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-2784
> https://issues.apache.org/jira/browse/MESOS-2784
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add constexpr to C++11 whitelist
> 
> 
> Diffs
> -
> 
>   docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e 
> 
> Diff: https://reviews.apache.org/r/34835/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Review Request 35470: Fixed the flaky oversbuscription tests.

[Jie Yu]

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

Review request for mesos, Niklas Nielsen and Vinod Kone.


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


Repository: mesos


Description
---

Fixed the flaky oversbuscription tests.


Diffs
-

  src/tests/oversubscription_tests.cpp e7d94cecb4a668f634e94fb0aba95155dc827510 

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


Testing
---

make check

repeat the oversubscription tests for 100 times


Thanks,

Jie Yu

Re: Review Request 34361: converted hard-coded strings to consts

[Niklas Nielsen]

> On June 9, 2015, 6:25 p.m., Ben Mahler wrote:
> > src/tests/master_tests.cpp, lines 3031-3034
> > 
> >
> > Why bother with all this? Why not just have `"key1"`, `"value1"`, 
> > `"key2"`, `"value2"` inlined appropriately throughout these tests. Very 
> > straightforward to read.
> 
> Colin Williams wrote:
> I think the issue with the changes remaining is that the test depends on 
> the same value occurring in several places. By consolidating to a variable 
> it's no longer possible for these values to get out of sync.
> 
> Niklas Nielsen wrote:
> Colin: exactly
> 
> Ben: We have label tests three places; in the master, slave and in the 
> modules and they use the same "foo", "bar", "baz", "qux" key value pairs.
> The idea was to centralize them, so they don't get out of date and to 
> avoid code duplication.
> 
> Does that make sense?
> 
> Ben Mahler wrote:
> Well, then let's just keep it simple and get rid of these special strings 
> by just making the tests use "key1", "value1", "key2", "value2", etc.
> 
> I'm not following your code duplication point, this introduces more code 
> (now there are additional global constants, which we like to avoid if 
> unnecessary).
> 
> Also, each test is ideally independent, why does the master's test for 
> labels affect the slave's test for labels? Let's say I need a test with 5 
> labels, you want to have 5x2=10 global constants to address this?
> 
> Niklas Nielsen wrote:
> Try to see how testLabelKey and testLabelValue was used previously and 
> "foo", "bar", "qux" was used on others and I created this ticket to unify the 
> way we do this, along with seeing these key value pair being created in the 
> slave and master tests.
> 
> Dispite the current patch, I do think we can simply and unify the test 
> label creation. Maybe create a CREATE_LABEL_PAIR(id) to make it more obvious 
> which key pairs are being tested. The comments in the test code need to carry 
> a bunch of context, to make sense out of the label transformations 
> (especially across the library border for the hooks tests).
> 
> This is all in spirit of reducing the tech debt we introduced. We are on 
> the same page on not introducing more lines/key pairs than before. Let us 
> iterate on this and get back to you.
> 
> Colin Williams wrote:
> Ben: I'm more in agreement with your sentiment, I'm not sure I see the 
> point of unifying label names into a centralized variable that aren't at all 
> related.
> 
> Niklas: I was a little confused by the original task description, so I 
> thought a sample patch would be a good discussion point. I don't entirely 
> understand the tech debt you're trying to solve. For example, it seems like 
> you're asking to have some constants defined somewhere that are used by both 
> master_test and slave_test, but the the similarities between these two only 
> seem incidental. I'm concerned that creating something like 
> CREATE_LABEL_PAIR(id) would instead obfuscate the intent of the code.
> 
> Niklas Nielsen wrote:
> Okay - thinking about this; I am on board with "key1", "value1" etc.
> 
> Colin: the tech debt is that we have some inlined constants (like "foo", 
> "bar", etc) and some which are constants (which have to be kept in sync 
> between hooks modules and test body).
> The idea was to unify the way we name the key value pairs.
> 
> Do you want to update this ticket to reflect this, or come with a new 
> patch set which tackles streaming the two?

Ping :)


- Niklas


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


On June 8, 2015, 12:05 p.m., Colin Williams wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34361/
> ---
> 
> (Updated June 8, 2015, 12:05 p.m.)
> 
> 
> Review request for mesos and Niklas Nielsen.
> 
> 
> Bugs: MESOS-2637
> https://issues.apache.org/jira/browse/MESOS-2637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> converted hard-coded strings to consts
> 
> 
> Diffs
> -
> 
>   src/tests/hook_tests.cpp 3ffde6d 
>   src/tests/master_tests.cpp ba3858f 
>   src/tests/slave_tests.cpp acae497 
> 
> Diff: https://reviews.apache.org/r/34361/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Colin Williams
> 
>

Re: Review Request 34375: Removed use of namespace aliases.

[Ben Mahler]

> On May 19, 2015, 2:02 a.m., Ben Mahler wrote:
> > This might be a valid namespace alias use case that we hadn't considered, 
> > because there is no way to be able to write just `http::Response` 
> > otherwise, is there?
> > 
> > Seems quite verbose to write process::http everywhere, and on the otherhand 
> > just having `Request` or `Response` seems to miss the context of it being 
> > http, thoughts?
> 
> Michael Park wrote:
> > This might be a valid namespace alias use case that we hadn't considered
> 
> I actually think we should reconsider the overall approach of disallowing 
> aliases (e.g. type, namespace).
> > because there is no way to be able to write just http::Response 
> otherwise, is there?
> 
> We could do so by declaring `using namespace process;` if we wanted to.
> > Seems quite verbose to write process::http everywhere and on the 
> otherhand just having Request or Response seems to miss the context of it 
> being http
> 
> This is the entire point of aliases in general, which is why I think we 
> should reconsider the decision to disallow them.
> > thoughts?
> 
> From a offline discussion with BenH, it seems like the decision to 
> disallow type and namespace aliases to avoid inconsistencies in different 
> files. For example, one person could write `namespace http = process::http;` 
> and another person could write `namespace phttp = process::http;`.
> 
> I think the correct solution is to come up with naming guidelines to give 
> them good and consistent names, rather than disallowing the feature. For 
> example, we don't disallow functions just because "someone could possibly 
> give it a horrible name". We instead try to influence people to give their 
> functions meaningful names in a consistent manner. An example of such rule is 
> that we write `member()` rather than `getMember()` for our getter functions. 
> We don't simply disallow getter functions just because people could say them 
> inconsistently.
> 
> Anyway, given the current rules, I would push for us to be consistent 
> with no special cases. Especially since we have much longer names all over in 
> the codebase that we cope with. (e.g. `mesos::master::allocator::Allocator`, 
> `&master::allocator::MesosAllocatorProcess::deactivateFramework`, 
> `master::allocator::HierarchicalDRFAllocator`)
> 
> I'm definitely interested in starting the discussion around reconsidering 
> the rules and perhaps formalizing some of the naming rules around aliases, 
> but I also think that that should happen outside of this review, probably in 
> a JIRA ticket.

Sounds great Michael! Happy to help you get these aliases added, at least 
restricted to this use case initially. :)


- Ben


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


On June 13, 2015, 9:51 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34375/
> ---
> 
> (Updated June 13, 2015, 9:51 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 3049e057c87571f5db73ee0b14db8b47132b2dff 
>   src/slave/slave.cpp 9af69d8f0b28c9441c684886c52320378f9b2869 
>   src/tests/master_tests.cpp ba3858fb3026f2cc7af0523bd1a0c541eeafc34b 
>   src/tests/slave_tests.cpp 8dae6e0842c2bedddc13d1c036390e85c7a96df7 
> 
> Diff: https://reviews.apache.org/r/34375/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35467: Fix a comment in Slave.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35467]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 5:37 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35467/
> ---
> 
> (Updated June 15, 2015, 5:37 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 
> 
> Diff: https://reviews.apache.org/r/35467/diff/
> 
> 
> Testing
> ---
> 
> Comment fix.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 34835: Add constexpr to C++ whitelist

[Paul Brett]

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

(Updated June 15, 2015, 5:59 p.m.)


Review request for mesos, Benjamin Hindman, Dave Lester, Ian Downes, and Vinod 
Kone.


Changes
---

Address review comments


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


Repository: mesos


Description
---

Add constexpr to C++11 whitelist


Diffs (updated)
-

  docs/mesos-c++-style-guide.md 94107ed21c6f09349ce691f9f4d36b43bbbe809e 

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


Testing
---


Thanks,

Paul Brett

Re: Review Request 35411: Send oversubscribable resources during (re-)registration.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On June 13, 2015, 2:04 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35411/
> ---
> 
> (Updated June 13, 2015, 2:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Bugs: MESOS-2866
> https://issues.apache.org/jira/browse/MESOS-2866
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Send oversubscribable resources during (re-)registration.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.hpp 0df1b55791963fb4159b7ea5318d09dde4f7d8c7 
>   src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 
>   src/tests/oversubscription_tests.cpp 
> e8ae053dd9cd712e49bd2830e414b7a3d99c20ca 
> 
> Diff: https://reviews.apache.org/r/35411/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 35410: Minor cleanups to the slave.

[Jie Yu]

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

Ship it!


Ship It!

- Jie Yu


On June 13, 2015, 2:04 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35410/
> ---
> 
> (Updated June 13, 2015, 2:04 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Minor cleanups to the slave.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp b523c2fce50e56f4f94d55a9488f49c53452e4d4 
> 
> Diff: https://reviews.apache.org/r/35410/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Review Request 35467: Fix a comment in Slave.

[Michael Park]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 

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


Testing
---

Comment fix.


Thanks,

Michael Park

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34703, 30032]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 3:27 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/30032/
> ---
> 
> (Updated June 15, 2015, 3:27 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When serving a static file, libprocess returns the header `Last-Modified` 
> which is used by browsers to control Cache.
> When a http request arrives containing the header `If-Modified-Since`, a 
> response `304 Not Modified` is returned if the date in the request and the 
> modification time (as returned by doing `stat` in the file) coincide.
> Unit tests added.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> e47cc7afbc8110759edf25a2dc05d09eda25c417 
>   3rdparty/libprocess/src/process.cpp 
> f919b997287435381dbe34cb5bfdf73641ebeb23 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 660af45e7fd45bdf5d43ad9aa54477fd94f87058 
> 
> Diff: https://reviews.apache.org/r/30032/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 29406: Introduce libevent ssl socket.

[Jojy Varghese]

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



3rdparty/libprocess/include/process/address.hpp


There are a few "if family == INET" (or similar) in the code. By 
specializing the net address structures on FAMILY, we will get rid of them. As 
most of the network structures (socket for example)  are classified based on 
family, this should naturally fit into the overall scheme.


- Jojy Varghese


On June 13, 2015, 9:23 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/29406/
> ---
> 
> (Updated June 13, 2015, 9:23 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Bugs: MESOS-1913
> https://issues.apache.org/jira/browse/MESOS-1913
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Requires:
> configure --enable-libevent --enable-libevent-socket --enable-ssl
> New environment variables:
> ```
> SSL_ENABLED=(false|0,true|1)
> SSL_CERT_FILE=(path to certificate)
> SSL_KEY_FILE=(path to key)
> SSL_VERIFY_CERT=(false|0,true|1)
> SSL_REQUIRE_CERT=(false|0,true|1)
> SSL_VERIFY_DEPTH=(4)
> SSL_CA_DIR=(path to CA directory)
> SSL_CA_FILE=(path to CA file)
> SSL_CIPHERS=(accepted ciphers separated by ':')
> SSL_ENABLE_SSL_V2=(false|0,true|1)
> SSL_ENABLE_SSL_V3=(false|0,true|1)
> SSL_ENABLE_TLS_V1_0=(false|0,true|1)
> SSL_ENABLE_TLS_V1_1=(false|0,true|1)
> SSL_ENABLE_TLS_V1_2=(false|0,true|1)
> ```
> 
> Only TLSV1.2 is enabled by default.
> Use the `ENABLE_SSL_V*` and `ENABLE_TLS_V*` environment variables to open up 
> more protocols.
> Use the `SSL_CIPHERS` environment variable to restrict or open up the 
> supported ciphers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 489ce359f383d819299335cbaa8c95724b0c6ac2 
>   3rdparty/libprocess/include/process/address.hpp 
> 729f5cd7ea981e43a33c1fe9d99d58b906a31158 
>   3rdparty/libprocess/include/process/socket.hpp 
> b8c2274de535ac473e49a09165b601c96d3ebe8b 
>   3rdparty/libprocess/src/libevent.hpp 
> f6cc72178613a30446629532a773afccfd404212 
>   3rdparty/libprocess/src/libevent.cpp 
> fb038597358135a06c1927d079cb7cb09fea7452 
>   3rdparty/libprocess/src/libevent_ssl_socket.hpp PRE-CREATION 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp PRE-CREATION 
>   3rdparty/libprocess/src/openssl.hpp PRE-CREATION 
>   3rdparty/libprocess/src/openssl.cpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> aadd7bb0ae12b93336900c76d8d5aaa4421ea198 
>   3rdparty/libprocess/src/socket.cpp 0e1cebb19e21c706b152d35a0b8722924c971a35 
> 
> Diff: https://reviews.apache.org/r/29406/diff/
> 
> 
> Testing
> ---
> 
> make check (uses non-ssl socket)
> benchmarks using ssl sockets
> master, slave, framework, webui launch with ssl sockets
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35179]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 3:25 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> ---
> 
> (Updated June 15, 2015, 3:25 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
> https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> ---
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35438]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 2:28 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 15, 2015, 2:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35353: Smaller fixes on libprocess firewall

[Alexander Rojas]

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

(Updated June 15, 2015, 5:51 p.m.)


Review request for mesos, Ben Mahler and Till Toenshoff.


Changes
---

Removes unused headers.


Repository: mesos


Description
---

See summary


Diffs (updated)
-

  3rdparty/libprocess/include/process/firewall.hpp 
16ed852d07344e53c6f9d30d4cd7d99e07c6606d 
  3rdparty/libprocess/include/process/process.hpp 
6a0b21d67912a40e0ec3220fdb250930be1979b2 
  3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 
  3rdparty/libprocess/src/tests/process_tests.cpp 
660af45e7fd45bdf5d43ad9aa54477fd94f87058 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 35463: Fix typos in stout README.

[Till Toenshoff]

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

Ship it!


Ship It!

- Till Toenshoff


On June 15, 2015, 3:40 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35463/
> ---
> 
> (Updated June 15, 2015, 3:40 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/README.md 
> 6fc09d6d3cc80c7155a6edc76467c765b160a465 
> 
> Diff: https://reviews.apache.org/r/35463/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 35463: Fix typos in stout README.

[Joris Van Remoortere]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/README.md 
6fc09d6d3cc80c7155a6edc76467c765b160a465 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 30032: Added support for cache control in libprocess when dealing with static files.

[Alexander Rojas]

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

(Updated June 15, 2015, 5:27 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

User `Time::create` to create time objects. Removes unnecessary code.


Bugs: mesos-708
https://issues.apache.org/jira/browse/mesos-708


Repository: mesos


Description
---

When serving a static file, libprocess returns the header `Last-Modified` which 
is used by browsers to control Cache.
When a http request arrives containing the header `If-Modified-Since`, a 
response `304 Not Modified` is returned if the date in the request and the 
modification time (as returned by doing `stat` in the file) coincide.
Unit tests added.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
e47cc7afbc8110759edf25a2dc05d09eda25c417 
  3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 
  3rdparty/libprocess/src/tests/process_tests.cpp 
660af45e7fd45bdf5d43ad9aa54477fd94f87058 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 34703: Added stream manipulators for the Time object.

[Alexander Rojas]

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

(Updated June 15, 2015, 5:26 p.m.)


Review request for mesos, Bernd Mathiske, Michael Park, Nikita Vetoshkin, and 
Till Toenshoff.


Changes
---

mpark's comments.
Changes serialization from "UTC" to "GMT" to get around a bug in osx (see man 
strptime)


Bugs: mesos-708
https://issues.apache.org/jira/browse/mesos-708


Repository: mesos


Description
---

Adds some manipulator classes which allows formatting Time objects to ostreams.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am c781f6ca38d87c47b4bdadf5ac2f59a02dd8c73c 
  3rdparty/libprocess/include/process/time.hpp 
c5ab2a3cfa83590eb6612152ae365dd67f51cea9 
  3rdparty/libprocess/src/tests/time_tests.cpp 
be314182c65c05d439b81aa5248a71d93f6f0a0b 
  3rdparty/libprocess/src/time.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Anand Mazumdar]

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

(Updated June 15, 2015, 3:25 p.m.)


Review request for mesos, Adam B and Cody Maloney.


Changes
---

Address Adam's review comments.


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


Repository: mesos


Description
---

This change takes an un-complicated/naive route ( no trimming of values etc ) 
at making path::join(...) variadic mainly in order to preserve the earlier 
over-loaded join functionality.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
  3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
cf354125687e0f60b6d5b105f19d75e4436f21bf 

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


Testing
---

make check + added some additional tests.


Thanks,

Anand Mazumdar

Re: Review Request 35455: Update 'Getting Started' documentation.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35455]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 12:58 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35455/
> ---
> 
> (Updated June 15, 2015, 12:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 
> 
> Diff: https://reviews.apache.org/r/35455/diff/
> 
> 
> Testing
> ---
> 
> Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 
> 6.6` and `CentOS 7.1` docker images.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

(Updated June 15, 2015, 7:28 a.m.)


Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
Vinod Kone.


Changes
---

Addressed Till's review.


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske

Re: Review Request 35433: CHECK that checkpointed resources exist on the slave.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35433]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 12:39 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35433/
> ---
> 
> (Updated June 15, 2015, 12:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> No bug was observed (yet), but realized I forgot about this in the dynamic 
> reservations patches.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 
> 
> Diff: https://reviews.apache.org/r/35433/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35179: MESOS-1733 Variadic Path Join

[Adam B]

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


Looks good. Besides some style nits, I'd also like you to remove the 
single-argument join and put the REQUIRE macro into a separate patch.


3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Also #include  // std::forward



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Why is this included? I don't think you're actually using enable_if in this 
file anymore.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Remove blank line



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Why do we need this single-element join version? Seems like the two-element 
version would be the sensible base case.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


This can fit on one line (<=80 char), so let's do so.



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Remove tab/indentation please



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Should only indent 2 spaces



3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp


Two blank lines between function implementations, please.



3rdparty/libprocess/3rdparty/stout/include/stout/require.hpp


This macro is unused in this patch. While it may be valuable, let's split 
it off into a separate patch to be reviewed separately. Ideally we would 
introduce the macro along with another patch that actually uses it.



3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp


Would it be worthwhile to also test:
path::join("ab", "/", "/")
path::join("/", "", "/ab")



3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp


I don't think a single-argument join makes any sense. We didn't support one 
before, and I would expect such a call to fail at compile-time. Please remove.


- Adam B


On June 12, 2015, 12:04 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35179/
> ---
> 
> (Updated June 12, 2015, 12:04 p.m.)
> 
> 
> Review request for mesos, Adam B and Cody Maloney.
> 
> 
> Bugs: MESOS-1733
> https://issues.apache.org/jira/browse/MESOS-1733
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change takes an un-complicated/naive route ( no trimming of values etc ) 
> at making path::join(...) variadic mainly in order to preserve the earlier 
> over-loaded join functionality.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
>   3rdparty/libprocess/3rdparty/stout/include/stout/require.hpp PRE-CREATION 
>   3rdparty/libprocess/3rdparty/stout/tests/path_tests.cpp 
> cf354125687e0f60b6d5b105f19d75e4436f21bf 
> 
> Diff: https://reviews.apache.org/r/35179/diff/
> 
> 
> Testing
> ---
> 
> make check + added some additional tests.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 34392: Added a method to Path which returns the modification time of the represented path.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [34392]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 11:34 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34392/
> ---
> 
> (Updated June 15, 2015, 11:34 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
> Michael Park, and Till Toenshoff.
> 
> 
> Bugs: mesos-708
> https://issues.apache.org/jira/browse/mesos-708
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
> d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 
> 
> Diff: https://reviews.apache.org/r/34392/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 35455: Update 'Getting Started' documentation.

[Michael Park]

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

(Updated June 15, 2015, 12:58 p.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
---

`s/#/$/`


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 

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


Testing
---

Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` 
and `CentOS 7.1` docker images.


Thanks,

Michael Park

Re: Review Request 35455: Update 'Getting Started' documentation.

[Michael Park]

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

(Updated June 15, 2015, 12:54 p.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
---

Add `tar` back, since we actually still need it.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 

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


Testing
---

Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` 
and `CentOS 7.1` docker images.


Thanks,

Michael Park

Re: Review Request 35433: CHECK that checkpointed resources exist on the slave.

[Michael Park]

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



src/slave/slave.cpp


Introduce a new `REASON_` enum for this.



src/slave/slave.cpp


Same here


- Michael Park


On June 15, 2015, 12:39 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35433/
> ---
> 
> (Updated June 15, 2015, 12:39 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> No bug was observed (yet), but realized I forgot about this in the dynamic 
> reservations patches.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 
> 
> Diff: https://reviews.apache.org/r/35433/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35455: Update 'Getting Started' documentation.

[Michael Park]

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

(Updated June 15, 2015, 12:42 p.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
---

Since we don't download the `maven` binary and un-`tar` it, `tar` is no longer 
a utility package we require.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 

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


Testing
---

Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` 
and `CentOS 7.1` docker images.


Thanks,

Michael Park

Re: Review Request 35433: CHECK that checkpointed resources exist on the slave.

[Michael Park]

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

(Updated June 15, 2015, 12:39 p.m.)


Review request for mesos, Benjamin Hindman and Jie Yu.


Changes
---

Addressed BenH's comment. The slave now sends `TASK_LOST` status updates since 
the fact that the checkpointed resources are available on the slave is not a 
global invariant we try to guarantee.


Repository: mesos


Description
---

No bug was observed (yet), but realized I forgot about this in the dynamic 
reservations patches.


Diffs (updated)
-

  src/slave/slave.cpp 67732a40ef683cb0188425f0bba8fe8ab83e461c 

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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 35455: Update 'Getting Started' documentation.

[Michael Park]

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

(Updated June 15, 2015, 12:36 p.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
---

Addressed Till's comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 

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


Testing
---

Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` 
and `CentOS 7.1` docker images.


Thanks,

Michael Park

Re: Review Request 35455: Update 'Getting Started' documentation.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35455]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 12:20 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35455/
> ---
> 
> (Updated June 15, 2015, 12:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 
> 
> Diff: https://reviews.apache.org/r/35455/diff/
> 
> 
> Testing
> ---
> 
> Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 
> 6.6` and `CentOS 7.1` docker images.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35455: Update 'Getting Started' documentation.

[Till Toenshoff]

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

Ship it!



docs/getting-started.md


Not sure this makes sense for this chapter.



docs/getting-started.md


Also not yours but this does not seem to make any sense. OS X does not need 
Python3 to get installed just like the other distros dont need it. AFAIK we are 
fine with Python 2.7.x on all platforms. Could you please get rid of this part?



docs/getting-started.md


I know this isn't yours but it seems weird to mark C++, Java and Python as 
being code keywords, no?


- Till Toenshoff


On June 15, 2015, 12:20 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35455/
> ---
> 
> (Updated June 15, 2015, 12:20 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 
> 
> Diff: https://reviews.apache.org/r/35455/diff/
> 
> 
> Testing
> ---
> 
> Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 
> 6.6` and `CentOS 7.1` docker images.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35455: Update 'Getting Started' documentation.

[Michael Park]

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

(Updated June 15, 2015, 12:20 p.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
---

@Till mentioned that we don't need Python 3 for Mac OS X.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 

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


Testing
---

Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` 
and `CentOS 7.1` docker images.


Thanks,

Michael Park

Re: Review Request 35455: Update 'Getting Started' documentation.

[Michael Park]

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

(Updated June 15, 2015, 12:11 p.m.)


Review request for mesos, Benjamin Hindman and Till Toenshoff.


Changes
---

Fixed copy-paste error.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 

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


Testing
---

Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` 
and `CentOS 7.1` docker images.


Thanks,

Michael Park

Re: Review Request 34392: Added a method to Path which returns the modification time of the represented path.

[Alexander Rojas]

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

(Updated June 15, 2015, 1:34 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Joerg Schad, 
Michael Park, and Till Toenshoff.


Changes
---

Till's comments.


Bugs: mesos-708
https://issues.apache.org/jira/browse/mesos-708


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/path.hpp 
d4df6502d1297ea3ad8e2a1e3bb16ea9d7c7913c 

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


Testing
---

make check


Thanks,

Alexander Rojas

Review Request 35455: Update 'Getting Started' documentation.

[Michael Park]

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

Review request for mesos, Benjamin Hindman and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  docs/getting-started.md af679665434f38dc999f519b985b5ddb4f782ff8 

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


Testing
---

Followed the instructions line by line with fresh `Ubuntu 14.04`, `CentOS 6.6` 
and `CentOS 7.1` docker images.


Thanks,

Michael Park

Re: Review Request 35354: Smaller fixes in libprocess firewall initialization

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35353, 35354]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 9:24 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35354/
> ---
> 
> (Updated June 15, 2015, 9:24 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294 
>   src/messages/flags.hpp 41be419ba5593a600aa0c6c411210fa4faa829a8 
>   src/slave/main.cpp c379243e01919a5ab30bb9dea1b738665ba4e746 
> 
> Diff: https://reviews.apache.org/r/35354/diff/
> 
> 
> Testing
> ---
> 
> Manual checks && make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 34943: Added validation to flags.

[Bernd Mathiske]

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



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


Could we use some more concrete typing like this here?

std::function(Option)>&& validate

Similar types could then also be used in all other places where "F&&" 
appears below.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


I wonder if we need this comment at all.  What it says seems rather 
obvious. You cannot assign a function value that does not match the given 
signature.

It seems that this comment is a remnant, because a less trivial variant of 
it follows further down below.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp


What if T1 is Path? Then fetch() only parses. BTW: what does this mean 
then? This does not seem to be explained in fetch.hpp or anywhere else.


- Bernd Mathiske


On June 14, 2015, 6:55 a.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34943/
> ---
> 
> (Updated June 14, 2015, 6:55 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also refactored existing 'lambda::bind' arguments to use C++11 lambdas, 
> enabling us to get rid of our "loader" and "stringifier" functors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
> 6ac2f04a6a1d8db1725972a3b4b46a0dd734566f 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flag.hpp 
> 87606d860dea3235ec70d127d3379065d42dc90b 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp 
> ee855da6128c2d671eb2fc7eaa2c0aab2341aebb 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/loader.hpp 
> 51d3ab020bd2bae1f12b66cac0da5383c813d5d7 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/stringifier.hpp 
> fda5ae1328a23a4371475652f921341dce7448d5 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> 80450185f60c5b273face490e0bb9e695b0cb984 
> 
> Diff: https://reviews.apache.org/r/34943/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35438]

All tests passed.

- Mesos ReviewBot


On June 15, 2015, 8:52 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 15, 2015, 8:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35353: Smaller fixes on libprocess firewall

[Alexander Rojas]

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

(Updated June 15, 2015, 11:24 a.m.)


Review request for mesos, Ben Mahler and Till Toenshoff.


Changes
---

till's comments.


Repository: mesos


Description
---

See summary


Diffs (updated)
-

  3rdparty/libprocess/include/process/firewall.hpp 
16ed852d07344e53c6f9d30d4cd7d99e07c6606d 
  3rdparty/libprocess/include/process/process.hpp 
6a0b21d67912a40e0ec3220fdb250930be1979b2 
  3rdparty/libprocess/src/process.cpp f919b997287435381dbe34cb5bfdf73641ebeb23 
  3rdparty/libprocess/src/tests/process_tests.cpp 
660af45e7fd45bdf5d43ad9aa54477fd94f87058 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 35354: Smaller fixes in libprocess firewall initialization

[Alexander Rojas]

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

(Updated June 15, 2015, 11:24 a.m.)


Review request for mesos, Ben Mahler and Till Toenshoff.


Changes
---

till's comments.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/main.cpp 1c33e3bf7baae45b1671f9566b4993f39d9f1294 
  src/messages/flags.hpp 41be419ba5593a600aa0c6c411210fa4faa829a8 
  src/slave/main.cpp c379243e01919a5ab30bb9dea1b738665ba4e746 

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


Testing
---

Manual checks && make check


Thanks,

Alexander Rojas

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Till Toenshoff]

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

Ship it!



src/tests/fetcher_cache_tests.cpp


Would it make sense here to add the underlying failure reason here any 
everywhere else, if available?

```
return Error("Failed to wait for resource offers: " + (offers.isFailed ? 
offers.failure() : "discarded"));
```



src/tests/fetcher_cache_tests.cpp


See above.



src/tests/fetcher_cache_tests.cpp


Can we get this comment into some more explicit form that also follows our 
styleguide?


- Till Toenshoff


On June 15, 2015, 8:52 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 15, 2015, 8:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Till Toenshoff, Timothy Chen, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

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

(Updated June 15, 2015, 1:51 a.m.)


Review request for mesos, Adam B, Benjamin Hindman, Till Toenshoff, and Timothy 
Chen.


Changes
---

Added TODO regarding the "15 seconds" naked constant when awaiting offers.


Repository: mesos


Description
---

Follow up to RR https://reviews.apache.org/r/35247/, which was not good enough, 
fixed only one of two problems.

Using DeclineOffers() instead of Return() should make the master resend offers 
so we can launch tasks. See line 205 below.

Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but we 
return a Try instead and follow call sites with EXPECT_SOME(task(s)).


Diffs (updated)
-

  src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 

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


Testing
---

make check


Thanks,

Bernd Mathiske

Re: Review Request 35438: Fixed fetcher cache test race for resource offers when starting tasks and changed corresponding CHECK to EXPECT.

[Bernd Mathiske]

> On June 14, 2015, 11:45 p.m., Timothy Chen wrote:
> > src/tests/fetcher_cache_tests.cpp, line 358
> > 
> >
> > Does this mean when the default changes we need to change this value 
> > too?
> > 
> > Also what's the rationale making it a Error/Try? Is this because a 
> > timeout can be a valid case here?

Since the default is a naked number in AWAIT_..., we would have to change 
library code, first, to improve this. I'll insert a TODO.

Many such test support macros are geared towards inlining all code into every 
test. Having local returns of type void, they don't work inside functions that 
do not return void - and they are incapable of ending the whole test from 
within nested calls.

So, in consequence, the strategy to uplevel errors into call level zero and use 
an EXPECT_... macro there seems right.

Another alternative is avoiding procedural abstraction, which is common in our 
(test) code base.


- Bernd


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


On June 14, 2015, 6:54 a.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35438/
> ---
> 
> (Updated June 14, 2015, 6:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Follow up to RR https://reviews.apache.org/r/35247/, which was not good 
> enough, fixed only one of two problems.
> 
> Using DeclineOffers() instead of Return() should make the master resend 
> offers so we can launch tasks. See line 205 below.
> 
> Following Jie's suggestion, no more CHECK_READY inside launchTask(s)(), but 
> we return a Try instead and follow call sites with EXPECT_SOME(task(s)).
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp 8bd5dd847fb189d0eeeaa760d3ec8ce3af1c2392 
> 
> Diff: https://reviews.apache.org/r/35438/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>