> On Jan. 16, 2013, 8:07 p.m., Ben Mahler wrote: > > src/slave/cgroups_isolation_module.cpp, line 429 > > <https://reviews.apache.org/r/8535/diff/4/?file=240998#file240998line429> > > > > What is path? > > > > I think this should be renamed.. to pidCheckpointPath? > > > > For that matter, isn't the bool redundant? > > checkpoint == pidCheckpointPath.isSome(), right? > > > > ditto on this comment in each isolation module
I used 'path' everywhere, to indicate where one has to checkpoint the data. I think its un-ambigous? Me and BeH had discussions on whether we want to use 'checkpoint' or just infer based on path as you suggested. We decided on being explicit to make the api clearer. Also, fwiw, in further downstream reviews the 'checkpoint' parameter has been removed and we directly get it from frameworkInfo. > On Jan. 16, 2013, 8:07 p.m., Ben Mahler wrote: > > src/tests/slave_state_tests.cpp, line 183 > > <https://reviews.apache.org/r/8535/diff/4/?file=241010#file241010line183> > > > > You also want to check that this is the result is true, right? > On Jan. 16, 2013, 8:07 p.m., Ben Mahler wrote: > > src/messages/messages.proto, line 263 > > <https://reviews.apache.org/r/8535/diff/4/?file=240996#file240996line263> > > > > I can't give a shipit if this is required, you'll have to either: > > 1. Convince me it's ok to leave this required. > > 2. Document an upgrade plan that allows for this to be required. > > > > You might do 1 by telling me that this message is only contained within > > the slave, and never hits the master? > > Will the slave and executors be rolled to have this new proto at the > > same time? Good catch. Decided to actually get rid of checkpointing this pid. So, we no longer need this field in the protobuf. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/8535/#review15402 ----------------------------------------------------------- On Dec. 19, 2012, 1:32 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/8535/ > ----------------------------------------------------------- > > (Updated Dec. 19, 2012, 1:32 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Checkpoints slave id, framework pid, executor pids (libprocess, execed, > forked) and task info. > > > Diffs > ----- > > src/exec/exec.cpp 821a94fab1f5969183ecf9e28d7b6bc10920db24 > src/messages/messages.proto 815fcbbcb4a8643f50950a294cedf7281b2a187f > src/slave/cgroups_isolation_module.hpp > 669efa14ba2603764aa68ae19a44e79dbfdec192 > src/slave/cgroups_isolation_module.cpp > 0f2975d1adf874dba4d0a539eb5c99233cef6e6b > src/slave/flags.hpp 39e57f4104ee7a1538436ebbb9493581e28c99dd > src/slave/isolation_module.hpp b962365ebeddd047896a66b02a327aa26ae323d3 > src/slave/lxc_isolation_module.hpp 2bc844f491befbe588965da2ada7cfcef0b6f0a4 > src/slave/lxc_isolation_module.cpp 30cff2a49339bb07030727d30352536a0a22d58c > src/slave/process_based_isolation_module.hpp > f1817192582e3646f8dcf17934ba7998829e8fd6 > src/slave/process_based_isolation_module.cpp > 3d50a4b652e4e09dd57e744e408c8fb79ff3fbf5 > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 > src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018 > src/slave/state.hpp c1e4c782380b0076313d2216c40e86774050d445 > src/slave/state.cpp 4d4c2470c44fe630ec0694ee937131b4f0aafc4e > src/tests/protobuf_io_tests.cpp 979efac0f28c3d361f5c347f15933d89d9356bc9 > src/tests/slave_state_tests.cpp 1136ea93cffb1483458edad2605b0b4f83b61c44 > src/tests/status_update_manager_tests.cpp PRE-CREATION > src/tests/utils.hpp be457117515ee727af101370b26bf9188afb8f45 > > Diff: https://reviews.apache.org/r/8535/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
