Re: Review Request 35635: Formatting cleanup in the Slave.

> On June 19, 2015, 12:40 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, lines 3154-3159 > > > > > > Is this really better? Also we have a lot of code following this old > > style, I'm hesitant to introduce inconsi

Re: Review Request 35635: Formatting cleanup in the Slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35635/ --- (Updated June 19, 2015, 2:12 a.m.) Review request for mesos, Benjamin Hindman a

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/#review88478 --- Ship it! src/tests/hierarchical_allocator_tests.cpp (line 984)

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/ --- (Updated June 19, 2015, 1:11 a.m.) Review request for mesos, Ben Mahler and Vin

Re: Review Request 35433: Sent StatusUpdates if checkpointed resources don't exist on the slave.

> On June 18, 2015, 5:27 p.m., Jie Yu wrote: > > src/slave/slave.cpp, line 1409 > > > > > > Have we decided to not capture temp variable by const ref? Maybe you > > want to do a sweep to fix that in this file? I fix

Re: Review Request 35638: Removed const-ref to temporaries from the Slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35638/ --- (Updated June 19, 2015, 12:43 a.m.) Review request for mesos, Benjamin Hindman,

Review Request 35638: Removed const-ref to temporaries from the Slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35638/ --- Review request for mesos, Benjamin Hindman, Jie Yu, and Joris Van Remoortere. R

Re: Review Request 35433: Sent StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 19, 2015, 12:42 a.m.) Review request for mesos, Alexander Ruklets

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 19, 2015, 12:42 a.m.) Review request for mesos, Alexander Ruklets

Re: Review Request 35635: Formatting cleanup in the Slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35635/#review88475 --- src/slave/slave.cpp (lines 2904 - 2905)

Re: Review Request 35635: Formatting cleanup in the Slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35635/#review88474 --- The following are the list of incorrect styles that were fixed in th

Review Request 35635: Formatting cleanup in the Slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35635/ --- Review request for mesos, Benjamin Hindman and Till Toenshoff. Repository: meso

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/#review88472 --- Ship it! Great, thanks Jie! src/tests/hierarchical_allocator_test

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/ --- (Updated June 19, 2015, 12:15 a.m.) Review request for mesos, Ben Mahler and Vi

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/ --- (Updated June 19, 2015, 12:15 a.m.) Review request for mesos, Ben Mahler and Vi

Re: Review Request 35632: Added queue size metric for the allocator.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35632/#review88463 --- Ship it! Ship It! - Jie Yu On June 18, 2015, 11:10 p.m., Ben Mah

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/ --- (Updated June 18, 2015, 11:14 p.m.) Review request for mesos, Ben Mahler and Vi

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/ --- (Updated June 18, 2015, 11:12 p.m.) Review request for mesos, Ben Mahler and Vi

Re: Review Request 35335: Fixed JSON output error TaskInfo executor_id in slave/http.cpp

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35335/ --- (Updated June 18, 2015, 11:11 p.m.) Review request for mesos, Benjamin Hindman

Re: Review Request 35335: Fixed JSON output error TaskInfo executor_id in slave/http.cpp

> On June 18, 2015, 7:11 p.m., Joris Van Remoortere wrote: > > Let's see if we can augment the test to force the task into the pending > > state when we query state.json. It seems that queued_tasks is difficult to test because it is hard to queue tasks in the executor without them being immedi

Review Request 35632: Added queue size metric for the allocator.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35632/ --- Review request for mesos and Jie Yu. Bugs: MESOS-2893 https://issues.apache

Re: Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/ --- (Updated June 18, 2015, 11:02 p.m.) Review request for mesos, Ben Mahler and Vi

Review Request 35631: Added a performance benchmark for hierarchical allocator addSlave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35631/ --- Review request for mesos, Ben Mahler and Vinod Kone. Bugs: MESOS-2892 https

Re: Review Request 35585: Updated Isolator to return required namespaces.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35585/#review88442 --- A few high level comments: 1) Let's use an enum instead of an unstru

Review Request 35625: Fixing a typo

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35625/ --- Review request for mesos. Repository: mesos Description --- Fixing a typ

Re: Review Request 35622: Adding a link to example framework implementations (RENDLER)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35622/ --- (Updated June 18, 2015, 9:29 p.m.) Review request for mesos and Joris Van Remoo

Re: Review Request 35611: Added initial doxygen documentation for stout flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35611/#review88438 --- Bad patch! Reviews applied: [34943, 35611] Failed command: ./suppo

Re: Review Request 35622: Adding a link to example framework implementations (RENDLER)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35622/ --- (Updated June 18, 2015, 9:24 p.m.) Review request for mesos and Joris Van Remoo

Re: Review Request 35405: Enable deleting MasterDetector in MesosSchedulerDriver::join.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35405/#review88437 --- src/sched/sched.cpp (line 1659)

Re: Review Request 35622: Adding a link to example framework implementations (RENDLR)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35622/ --- (Updated June 18, 2015, 9:18 p.m.) Review request for mesos and Joris Van Remoo

Re: Review Request 35622: Adding a link to example framework implementations (RENDLER)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35622/ --- (Updated June 18, 2015, 9:18 p.m.) Review request for mesos and Joris Van Remoo

Re: Review Request 35622: Adding a link to example framework implementations (RENDLR)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35622/ --- (Updated June 18, 2015, 9:15 p.m.) Review request for mesos and Joris Van Remoo

Re: Review Request 35622: Adding a link to example framework implementations (RENDLR)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35622/#review88436 --- docs/app-framework-development-guide.md (line 9)

Review Request 35622: Adding a link to example framework implementations (RENDLR)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35622/ --- Review request for mesos and Joris Van Remoortere. Repository: mesos Descript

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

> On June 18, 2015, 5:21 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, lines 1418-1420 > > > > > > The only logic question I have here now is how come we don't need the > > following code even though we use

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 18, 2015, 7:47 p.m.) Review request for mesos, Alexander Rukletso

Re: Review Request 35285: Rename Option::get(const T& _t) to getOrElse() and refactor original functions (mesos)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35285/#review88429 --- Hey Mark, Can you make the dependency chain purely linear so the rev

Re: Review Request 35287: Rename Option::get(const T& _t) to getOrElse() and refactor original functions (stout)

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35287/#review88428 --- Thanks for taking this on Mark! Would you mind rebasing this chain o

Re: Review Request 35571: Adding ability to decode JSON from ZK

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88423 --- Thanks Marco, want to see this polished off properly before a ship i

Re: Review Request 35561: Updated process::subprocess to replace environment.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35561/ --- (Updated June 18, 2015, 6:48 p.m.) Review request for mesos and Till Toenshoff.

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 18, 2015, 6:45 p.m.) Review request for mesos, Alexander Rukletso

Re: Review Request 35571: Adding ability to decode JSON from ZK

> On June 18, 2015, 1:08 a.m., Ben Mahler wrote: > > src/master/detector.cpp, lines 444-447 > > > > > > Our logging messages do not end with periods, please do a sweep :) > > > > Ditto for aligning on the << o

Re: Review Request 35561: Updated process::subprocess to replace environment.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35561/ --- (Updated June 18, 2015, 6:39 p.m.) Review request for mesos and Till Toenshoff.

Re: Review Request 35561: Updated process::subprocess to replace environment.

> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote: > > 3rdparty/libprocess/src/subprocess.cpp, line 332 > > > > > > Aren't we leaking this one in the parent process? > > Benjamin Hindman wrote: > We'll be exec'i

Re: Review Request 35561: Updated process::subprocess to replace environment.

> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote: > > 3rdparty/libprocess/src/subprocess.cpp, line 332 > > > > > > Aren't we leaking this one in the parent process? > > Benjamin Hindman wrote: > We'll be exec'i

Re: Review Request 35561: Updated process::subprocess to replace environment.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35561/#review88419 --- 3rdparty/libprocess/src/subprocess.cpp (lines 329 - 340)

Re: Review Request 35571: Adding ability to decode JSON from ZK

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88418 --- Ship it! Please definitely write a test (in a follow up review) to

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

> On June 18, 2015, 5:21 p.m., Benjamin Hindman wrote: > > src/slave/slave.cpp, lines 1418-1420 > > > > > > The only logic question I have here now is how come we don't need the > > following code even though we use

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

> On June 18, 2015, 5:27 p.m., Jie Yu wrote: > > src/slave/slave.cpp, lines 1403-1421 > > > > > > Hum, are you sending multiple status update if there are multiple > > 'resource's in 'checkpointedTaskResources'? My

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review88406 --- src/slave/slave.cpp (lines 1403 - 1421)

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review88405 --- src/slave/slave.cpp (lines 1418 - 1420)

Re: Review Request 34767: Added testing patterns doc.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34767/#review88399 --- Ship it! Only comment here is that we need to use ~~~ instead of ``

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

> On June 18, 2015, 5 p.m., Jie Yu wrote: > > Your latest diff does not look right. Sorry about that, rebased. - Michael --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review88392 ---

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

> On June 18, 2015, 5:02 p.m., Alexander Rukletsov wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp, lines 104-112 > > > > > > Ditto. Yeah, sorry about that. Rebased. - Michael --

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

> On June 18, 2015, 5:02 p.m., Alexander Rukletsov wrote: > > docs/mesos-frameworks.md, lines 29-30 > > > > > > Artifact? This was recently removed. Yeah, sorry about that. Rebased. - Michael --

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 18, 2015, 5:03 p.m.) Review request for mesos, Benjamin Hindman a

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review88393 --- Michael, there are some artifacts in the patch, could you please reb

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 18, 2015, 5:01 p.m.) Review request for mesos, Benjamin Hindman a

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review88392 --- Your latest diff does not look right. - Jie Yu On June 18, 2015,

Re: Review Request 35433: Send StatusUpdates if checkpointed resources don't exist on the slave.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 18, 2015, 4:54 p.m.) Review request for mesos, Benjamin Hindman a

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/ --- (Updated June 18, 2015, 4:54 p.m.) Review request for mesos, Benjamin Hindman a

Re: Review Request 34943: Added validation to flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/#review88386 --- Ship it! Ship It! - Bernd Mathiske On June 18, 2015, 8:34 a.m.,

Re: Review Request 35571: Adding ability to decode JSON from ZK

> On June 17, 2015, 11:52 p.m., Niklas Nielsen wrote: > > LGTM modulo wrapping at line 450-451. > > > > Would love to see a test before we ship this. > > > > @vinod - can you take a look at this too? i'll take a look now. - Vinod ---

Re: Review Request 34767: Added testing patterns doc.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34767/ --- (Updated June 18, 2015, 3:56 p.m.) Review request for mesos, Benjamin Hindman,

Re: Review Request 34943: Added validation to flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/#review88384 --- Ship it! Ship It! - Niklas Nielsen On June 18, 2015, 8:34 a.m.,

Re: Review Request 34943: Added validation to flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/ --- (Updated June 18, 2015, 3:34 p.m.) Review request for mesos and Niklas Nielsen.

Re: Review Request 35611: Added initial doxygen documentation for stout flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35611/ --- (Updated June 18, 2015, 3:33 p.m.) Review request for mesos and Bernd Mathiske.

Re: Review Request 35571: Adding ability to decode JSON from ZK

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 18, 2015, 3:31 p.m.) Review request for mesos, Niklas Nielsen and

Review Request 35611: Added initial doxygen documentation for stout flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35611/ --- Review request for mesos and Bernd Mathiske. Repository: mesos Description --

Re: Review Request 35571: Adding ability to decode JSON from ZK

> On June 18, 2015, 7:53 a.m., Benjamin Hindman wrote: > > Still a Ship It, just a minor nit from the revision. @BenH and @BenM; thanks for stepping in! I can get this committed today. - Niklas --- This is an automatically generated e-

Re: Review Request 35571: Adding ability to decode JSON from ZK

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88378 --- Ship it! Still a Ship It, just a minor nit from the revision. src

Re: Review Request 35571: Adding ability to decode JSON from ZK

> On June 18, 2015, 11:09 a.m., Benjamin Hindman wrote: > > src/master/detector.cpp, lines 468-469 > > > > > > You should always know this is an error because you're in the 'else' > > branch, so you can just do: > >

Re: Review Request 35571: Adding ability to decode JSON from ZK

> On June 18, 2015, 11:09 a.m., Benjamin Hindman wrote: > > This looks great Marco. Let's get this committed folks! Then please follow > > up with a test as asked by some of your reviewers. Oh, BTW - this code is actually tested by existing tests (in that, it doesn't break current functionalit

Re: Review Request 35571: Adding ability to decode JSON from ZK

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/ --- (Updated June 18, 2015, 2:46 p.m.) Review request for mesos, Niklas Nielsen and

Re: Review Request 35571: Adding ability to decode JSON from ZK

> On June 18, 2015, 11:09 a.m., Benjamin Hindman wrote: > > src/master/constants.hpp, line 102 > > > > > > Is there a way to capture deprecated with Doxygen? According to [this](http://www.stack.nl/~dimitri/doxygen/m

Re: Review Request 35566: Refactor executorEnvironment to take slave::Flags.

> On June 18, 2015, 1:01 a.m., Till Toenshoff wrote: > > src/tests/containerizer.cpp, line 124 > > > > > > This indention will very soon not comply with our styleguide anymore, > > no? No, it will, we still wrap the

Re: Review Request 35562: Removed unnecessary use of os::ExecEnv.

> On June 17, 2015, 8:32 p.m., Till Toenshoff wrote: > > src/slave/containerizer/mesos/launch.cpp, line 221 > > > > > > Does this TODO refer to a "clean environment" as being an environment > > without anything being

Re: Review Request 35561: Updated process::subprocess to replace environment.

> On June 17, 2015, 8:23 p.m., Till Toenshoff wrote: > > 3rdparty/libprocess/src/subprocess.cpp, line 332 > > > > > > Aren't we leaking this one in the parent process? We'll be exec'ing, so they'll get "freed" then. S

Re: Review Request 35405: Enable deleting MasterDetector in MesosSchedulerDriver::join.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35405/ --- (Updated June 18, 2015, 12:39 p.m.) Review request for mesos and Niklas Nielsen

Re: Review Request 34943: Added validation to flags.

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34943/ --- (Updated June 18, 2015, 12:29 p.m.) Review request for mesos and Niklas Nielsen

Re: Review Request 34943: Added validation to flags.

> On June 15, 2015, 10:17 a.m., Bernd Mathiske wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, line 146 > > > > > > Could we use some more concrete typing like this here? > > > > s

Re: Review Request 34943: Added validation to flags.

> On June 15, 2015, 11:48 p.m., Niklas Nielsen wrote: > > 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 >

Re: Review Request 34943: Added validation to flags.

> On June 15, 2015, 11:48 p.m., Niklas Nielsen wrote: > > 3rdparty/libprocess/3rdparty/stout/include/stout/flags/flags.hpp, lines > > 484-490 > > > > > > I am a bit torn whether we should copy this block so many place

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35433/#review88362 --- Ship it! I'm happy for you to pass None() as the TaskStatus reason

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35438/#review88361 --- Ship it! This looks good Bernd. And you should be able to commit th

Re: Review Request 35571: Adding ability to decode JSON from ZK

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35571/#review88359 --- Ship it! This looks great Marco. Let's get this committed folks! Th

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

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/34835/#review88352 --- docs/mesos-c++-style-guide.md (line 535)

Re: Review Request 35353: Smaller fixes on libprocess firewall

--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/35353/ --- (Updated June 18, 2015, 11:20 a.m.) Review request for mesos, Ben Mahler and Ti