----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6842/#review11450 -----------------------------------------------------------
rebase looks good! a more thorough review, have yet to look at the tests src/slave/slave.cpp <https://reviews.apache.org/r/6842/#comment24495> you moved this into protobuf utils, so kill this version src/slave/state.hpp <https://reviews.apache.org/r/6842/#comment24500> can you kill these from the header? any reason for exposing them, other than testing? I thought I would need some of these for file access, but I only would need the executor work directory from createUniqueExecutorWorkDirectory src/slave/state.hpp <https://reviews.apache.org/r/6842/#comment24501> two space separation on these guys, no? src/slave/state.hpp <https://reviews.apache.org/r/6842/#comment24515> comment all these guys src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24502> 80 line length limit on these src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24503> use the helper? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24508> any ideas for reusing the helpers on these * globs? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24506> ditto src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24511> s/rPath/runPath src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24509> s/result/run src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24510> can kill this var, and just use run.get() src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24512> s/tPath/taskPath src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24513> inline the basename here, instead of storing it in a variable? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24516> move this comment to the header src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24514> inline os::mkdir in the CHECK? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24517> move this down to where you use it src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24518> maybe strings::trim(result.get())? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24519> maybe strings::trim(result.get()) third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6842/#comment24496> can you move these new functions into the newly created stout/fs.hpp? third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6842/#comment24497> let's stay away from modifying the underlying char array of a string: http://stackoverflow.com/questions/5729203/modifying-underlying-char-array-of-a-c-string-object create a char* copy instead? third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6842/#comment24498> ditto third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6842/#comment24499> seems cleaner if we change this to a Try<list<string>> and have empty list indicate no matches - Ben Mahler On Sept. 12, 2012, 10:23 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6842/ > ----------------------------------------------------------- > > (Updated Sept. 12, 2012, 10:23 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > And so it begins... > > I am discarding the old review associated with this (previously called path: > https://reviews.apache.org/r/4944/), in favor of a fresh start. > > slave/state.{hpp,cpp} contains all the functions that deal with persistent > state of the slave. > > > This addresses bug MESOS-110. > https://issues.apache.org/jira/browse/MESOS-110 > > > Diffs > ----- > > src/Makefile.am b73a024 > src/common/protobuf_utils.hpp PRE-CREATION > src/slave/slave.hpp 7fbae8a > src/slave/slave.cpp 4ea1db1 > src/slave/state.hpp PRE-CREATION > src/slave/state.cpp PRE-CREATION > src/tests/slave_state_tests.cpp PRE-CREATION > third_party/libprocess/include/stout/format.hpp c746ad3 > third_party/libprocess/include/stout/os.hpp d08b0cb > > Diff: https://reviews.apache.org/r/6842/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
