> On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/slave.cpp, line 86 > > <https://reviews.apache.org/r/6842/diff/2/?file=153051#file153051line86> > > > > you moved this into protobuf utils, so kill this version
good catch. killed > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.hpp, line 39 > > <https://reviews.apache.org/r/6842/diff/2/?file=153052#file153052line39> > > > > 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 These would be useful outside testing too. For eg: in the forthcoming Status Update Manager code. > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.hpp, line 140 > > <https://reviews.apache.org/r/6842/diff/2/?file=153052#file153052line140> > > > > comment all these guys Done. Though, I'm now having doubts about the usefulness of the helper functions to read/write stuff. Added a TODO to revisit them after I integrate with StatusUpdateManager. > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 26 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line26> > > > > 80 line length limit on these actually, now that these were moved to the cpp file and we have helpers, decided to revert back to my old stye of composing paths. in my opinion, the intention is clearer with composition. > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 156 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line156> > > > > use the helper? oops, done > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 175 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line175> > > > > any ideas for reusing the helpers on these * globs? this will end up being a bunch templatized functions akin to format. i will punt on this till we move to c++11. > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 189 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line189> > > > > ditto ditto > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 201 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line201> > > > > s/result/run 'run' is used below. > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 207 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line207> > > > > can kill this var, and just use run.get() there are too many uses of run.get(), that made me want to extract it out, to make it cleaner > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 277 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line277> > > > > inline os::mkdir in the CHECK? see Ben's comment on my previous review (slave gc) regarding this. basically, since CHECK looks/acts like ASSERT, Ben wants to avoid having statements as argument. > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 332 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line332> > > > > maybe strings::trim(result.get())? we control the writes, so not needed? > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 371 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line371> > > > > maybe strings::trim(result.get()) ditto > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > third_party/libprocess/include/stout/os.hpp, line 191 > > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line191> > > > > can you move these new functions into the newly created stout/fs.hpp? i will do it once you commit it :) > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > third_party/libprocess/include/stout/os.hpp, line 195 > > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line195> > > > > 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? it looks ugly after the fix, but hopefully correctness trump ugliness :) > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > third_party/libprocess/include/stout/os.hpp, line 416 > > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line416> > > > > ditto ditto > On Sept. 13, 2012, 4:02 a.m., Ben Mahler wrote: > > third_party/libprocess/include/stout/os.hpp, line 739 > > <https://reviews.apache.org/r/6842/diff/2/?file=153056#file153056line739> > > > > seems cleaner if we change this to a Try<list<string>> and have empty > > list indicate no matches agreed, done - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6842/#review11450 ----------------------------------------------------------- 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 > >
