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

2015-06-16 Thread Michael Park
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35179/#review88059 --- Ship it! Ship It! - Michael Park On June 15, 2015, 11:23 p.m.,

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

2015-06-16 Thread Alexander Rojas
On June 16, 2015, 10:29 a.m., Bernd Mathiske wrote: 3rdparty/libprocess/src/time.cpp, line 41 https://reviews.apache.org/r/34703/diff/5/?file=984729#file984729line41 Why can't we use strfmt_l with a locale argument that matches the output we want? `strfmt_l` is available only on

Re: Review Request 35510: Introduced General and Markdown Documentation Style Guides.

2015-06-16 Thread Joerg Schad
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/ --- (Updated June 16, 2015, 2:03 p.m.) Review request for mesos and Bernd

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

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/ --- (Updated June 16, 2015, 2:24 p.m.) Review request for mesos, Bernd Mathiske,

Review Request 35508: Improve readability in post-review

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35508/ --- Review request for mesos, Ben Mahler and Vinod Kone. Repository: mesos

Re: Review Request 35508: Improve readability in post-review

2015-06-16 Thread Marco Massenzio
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35508/#review88082 --- Beauty being in the eye of the beholder, I'm not entirely sure that

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

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35473/#review88090 --- LGTM module Ben's comment and removing the (now obsolete) test

Re: Review Request 35510: Introduced General and Markdown Documentation Style Guides.

2015-06-16 Thread Joerg Schad
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/ --- (Updated June 16, 2015, 6:10 p.m.) Review request for mesos and Bernd

Re: Review Request 35509: Doxygen Style Guide Improvements.

2015-06-16 Thread Joerg Schad
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35509/ --- (Updated June 16, 2015, 6:11 p.m.) Review request for mesos and Bernd

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

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/#review88085 --- src/hook/manager.cpp (lines 96 - 98)

Re: Review Request 35510: Introduced General and Markdown Documentation Style Guides.

2015-06-16 Thread Joerg Schad
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/ --- (Updated June 16, 2015, 5:42 p.m.) Review request for mesos and Bernd

Re: Review Request 35508: Improve readability in post-review

2015-06-16 Thread Alexander Rojas
On June 16, 2015, 6:40 p.m., Marco Massenzio wrote: Beauty being in the eye of the beholder, I'm not entirely sure that I like the new version better (that is: assuming that the new and improved version is the bottom one?). Can you please make the description clearer by stating:

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

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30339/#review88088 --- Ship it! Ship It! - Niklas Nielsen On June 15, 2015, 3:26 p.m.,

Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.

2015-06-16 Thread Ian Downes
On June 4, 2015, 11:32 a.m., Chi Zhang wrote: src/tests/launch_tests.cpp, line 91 https://reviews.apache.org/r/31444/diff/6/?file=975951#file975951line91 for discussion: this requires these directories not existent in rootfs. should we specify requirements for the structure under

Re: Review Request 34721: Added QoS kill executor correction test.

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34721/#review88128 --- src/tests/mesos.hpp (line 205)

Re: Review Request 30643: Optionally specify executor for mesos execute.

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30643/#review88140 --- This is pretty hacky. Can you elaborate why this is beneficial for

Re: Review Request 35330: Capped number of parallel inspect instances on a docker ps call.

2015-06-16 Thread Jie Yu
On June 16, 2015, 10:02 p.m., Ben Mahler wrote: High level review, why don't we just have an abstraction for limiting the number of concurrent operations? You could keep this abstraction local to this file for now and consider pulling it out into libprocess. For example, rather

Review Request 35538: [MESOS-1988] Remove unused drop(...) overload

2015-06-16 Thread Anand Mazumdar
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35538/ --- Review request for mesos and Vinod Kone. Bugs: MESOS-1988

Re: Review Request 35330: Capped number of parallel inspect instances on a docker ps call.

2015-06-16 Thread Ben Mahler
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35330/#review88134 --- High level review, why don't we just have an abstraction for

Re: Review Request 35257: Decode network statistics from helper

2015-06-16 Thread Paul Brett
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35257/ --- (Updated June 16, 2015, 10:22 p.m.) Review request for mesos, Chi Zhang, Ian

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

2015-06-16 Thread Jie Yu
On June 16, 2015, 5:09 p.m., Niklas Nielsen wrote: src/master/allocator/sorter/drf/sorter.cpp, line 177 https://reviews.apache.org/r/35473/diff/2/?file=984869#file984869line177 This isn't just fixing the check's then but is introducing a fast path? Should we capture this in the

Re: Review Request 35330: Capped number of parallel inspect instances on a docker ps call.

2015-06-16 Thread Lily Chen
On June 13, 2015, 5:58 a.m., Timothy Chen wrote: src/docker/docker.cpp, line 834 https://reviews.apache.org/r/35330/diff/2/?file=983205#file983205line834 this too? doesn't fit on 80 lines On June 13, 2015, 5:58 a.m., Timothy Chen wrote: src/docker/docker.cpp, line 852

Re: Review Request 35330: Capped number of parallel inspect instances on a docker ps call.

2015-06-16 Thread Timothy Chen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35330/#review88133 --- src/docker/docker.cpp (line 887)

Re: Review Request 35538: [MESOS-1988] Remove unused drop(...) overload

2015-06-16 Thread Isabel Jimenez
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35538/#review88137 --- Ship it! Ship It! - Isabel Jimenez On June 16, 2015, 9:44 p.m.,

Re: Review Request 35229: Report per-container metrics for network bandwidth throttling to the slave.

2015-06-16 Thread Paul Brett
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35229/ --- (Updated June 16, 2015, 10 p.m.) Review request for mesos, Chi Zhang, Ian

Re: Review Request 34720: Added kill executor correction to slave.

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34720/#review88124 --- Ship it! LGTM. Some nits on logging. src/slave/slave.cpp (line

Re: Review Request 34720: Added kill executor correction to slave.

2015-06-16 Thread Jie Yu
On June 16, 2015, 9:22 p.m., Jie Yu wrote: LGTM. Some nits on logging. IN fact, can you consistently use `QoS correction KILL` (instead of QoS KILL correction) in the logging? - Jie --- This is an automatically generated e-mail. To

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

2015-06-16 Thread Paul Brett
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/ --- (Updated June 16, 2015, 8:27 p.m.) Review request for mesos, Chi Zhang, Ian

Re: Review Request 34135: Add filesystem/ isolators for persistent volumes.

2015-06-16 Thread Jiang Yan Xu
On June 2, 2015, 2:45 p.m., Timothy Chen wrote: src/slave/containerizer/isolators/filesystem/linux.cpp, line 95 https://reviews.apache.org/r/34135/diff/1/?file=957256#file957256line95 actually I'm wrong, I was reading the old style guide. The newest style guide we do put a space,

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

2015-06-16 Thread Paul Brett
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/ --- Review request for mesos, Chi Zhang, Ian Downes, Jie Yu, and Cong Wang. Bugs:

Re: Review Request 34721: Added QoS kill executor correction test.

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34721/ --- (Updated June 16, 2015, 1:43 p.m.) Review request for mesos, Bartek Plotka,

Re: Review Request 34719: Added REASON_EXECUTOR_PREEMPTED as status reason.

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34719/ --- (Updated June 16, 2015, 1:40 p.m.) Review request for mesos, Bartek Plotka,

Re: Review Request 34719: Added REASON_EXECUTOR_PREEMPTED as status reason.

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34719/#review88123 --- Ship it! include/mesos/mesos.proto (line 902)

Re: Review Request 34720: Added kill executor correction to slave.

2015-06-16 Thread Niklas Nielsen
On June 12, 2015, 3:49 p.m., Jie Yu wrote: src/slave/slave.cpp, lines 4203-4210 https://reviews.apache.org/r/34720/diff/5/?file=983931#file983931line4203 Can you add a TODO here saying that we may want to check if containerId matches or not due to race (i.e., qos controller

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

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/#review88145 --- Ship it! Great cleanup! Thanks Paul!

Re: Review Request 35538: [MESOS-1988] Remove unused drop(...) overload

2015-06-16 Thread Vinod Kone
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35538/#review88155 --- Ship it! Ship It! - Vinod Kone On June 16, 2015, 9:44 p.m.,

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

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/#review88147 --- src/slave/containerizer/isolators/network/port_mapping.cpp (line

Re: Review Request 35330: Capped number of parallel inspect instances on a docker ps call.

2015-06-16 Thread Timothy Chen
On June 16, 2015, 10:02 p.m., Ben Mahler wrote: High level review, why don't we just have an abstraction for limiting the number of concurrent operations? You could keep this abstraction local to this file for now and consider pulling it out into libprocess. For example, rather

Re: Review Request 30643: Optionally specify executor for mesos execute.

2015-06-16 Thread Vinod Kone
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30643/#review88151 --- Can you split this review into 1) adding support for a custom

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

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/#review88146 --- src/tests/port_mapping_tests.cpp (line 1587)

Re: Review Request 35494: Added format for template declaration in the styleguide.

2015-06-16 Thread Mesos ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35494/#review88181 --- Patch looks great! Reviews applied: [35494] All tests passed. -

Re: Review Request 35544: containerizer: statically initialize isolator factories

2015-06-16 Thread Mesos ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35544/#review88191 --- Patch looks great! Reviews applied: [35544] All tests passed. -

Re: Review Request 35510: Introduced General and Markdown Documentation Style Guides.

2015-06-16 Thread Mesos ReviewBot
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35510/#review88184 --- Bad patch! Reviews applied: [35364, 35509, 35510] Failed command:

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

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated June 17, 2015, 4:08 a.m.) Review request for mesos, Benjamin Hindman,

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

2015-06-16 Thread Paul Brett
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35536/ --- (Updated June 16, 2015, 11:44 p.m.) Review request for mesos, Chi Zhang, Ian

Re: Review Request 34719: Added REASON_EXECUTOR_PREEMPTED as status reason.

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34719/ --- (Updated June 16, 2015, 4:45 p.m.) Review request for mesos, Bartek Plotka,

Re: Review Request 34721: Added QoS kill executor correction test.

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34721/ --- (Updated June 16, 2015, 4:50 p.m.) Review request for mesos, Bartek Plotka,

Re: Review Request 34720: Added kill executor correction to slave.

2015-06-16 Thread Niklas Nielsen
On June 16, 2015, 2:22 p.m., Jie Yu wrote: src/slave/slave.cpp, line 4210 https://reviews.apache.org/r/34720/diff/6/?file=985948#file985948line4210 Shouldn't be 'executorId' here? Also, can you put single quotes for executorId since it's specifed by the user (i.e., might contain

Re: Review Request 34721: Added QoS kill executor correction test.

2015-06-16 Thread Niklas Nielsen
On June 16, 2015, 2:31 p.m., Jie Yu wrote: src/tests/mesos.hpp, line 205 https://reviews.apache.org/r/34721/diff/7/?file=985951#file985951line205 Can you instead add a overload for qosController only (similar to what we did for resourceEstimator above)? Done On June 16, 2015,

Re: Review Request 34721: Added QoS kill executor correction test.

2015-06-16 Thread Jie Yu
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34721/#review88169 --- Ship it! LGTM! src/tests/oversubscription_tests.cpp (line 762)

Re: Review Request 34721: Added QoS kill executor correction test.

2015-06-16 Thread Niklas Nielsen
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34721/ --- (Updated June 16, 2015, 5 p.m.) Review request for mesos, Bartek Plotka, Jie

Re: Review Request 35506: Excluding MarkDown files from the style hook

2015-06-16 Thread Isabel Jimenez
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35506/#review88176 --- Ship it! Ship It! - Isabel Jimenez On June 16, 2015, 11:24

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

2015-06-16 Thread Paul Brett
On June 16, 2015, 10:33 p.m., Jie Yu wrote: src/slave/containerizer/isolators/network/port_mapping.cpp, line 713 https://reviews.apache.org/r/35536/diff/1/?file=985924#file985924line713 I don't think you need to set this. It is a required field. Without it, the conversion to JSON

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

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/30032/ --- (Updated June 16, 2015, 9:53 a.m.) Review request for mesos, Benjamin Hindman,

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

2015-06-16 Thread 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.,

Review Request 35494: Added format for template declaration in the styleguide.

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35494/ --- Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-2705

Re: Review Request 35494: Added format for template declaration in the styleguide.

2015-06-16 Thread Till Toenshoff
On June 16, 2015, 8:13 a.m., Joerg Schad wrote: Actually I would really like to understand why we do it this way. Most other code I know omits the space. Still, as this seems to be common practice... That question should get raised on the JIRA to make things transparent. - Till

Re: Review Request 35364: Consistent code examples in doxygen style.

2015-06-16 Thread Bernd Mathiske
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35364/#review88045 --- Ship it! Ship It! - Bernd Mathiske On June 11, 2015, 1:39 p.m.,

Re: Review Request 35494: Added format for template declaration in the styleguide.

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35494/ --- (Updated June 16, 2015, 10:17 a.m.) Review request for mesos, Bernd Mathiske

Re: Review Request 35494: Added format for template declaration in the styleguide.

2015-06-16 Thread Joerg Schad
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35494/#review88039 --- Ship it! Actually I would really like to understand why we do it

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

2015-06-16 Thread Bernd Mathiske
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34703/#review88038 --- 3rdparty/libprocess/src/tests/time_tests.cpp (line 42)

Re: Review Request 35494: Added format for template declaration in the styleguide.

2015-06-16 Thread Michael Park
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35494/#review88054 --- Ship it! docs/mesos-c++-style-guide.md (line 64)

Review Request 35506: Excluding MarkDown files from the style hook

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35506/ --- Review request for mesos, Bernd Mathiske and Joerg Schad. Bugs: MESOS-2873

Re: Review Request 35494: Added format for template declaration in the styleguide.

2015-06-16 Thread Alexander Rojas
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35494/ --- (Updated June 16, 2015, 1:27 p.m.) Review request for mesos, Bernd Mathiske