> On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/common/protobuf_utils.hpp, line 37 > > <https://reviews.apache.org/r/6842/diff/1/?file=147306#file147306line37> > > > > just directly return? > > > > return (state == TASK_FINISHED || > > state == TASK_FAILED || > > state == TASK_KILLED || > > state == TASK_LOST); > > > >
done > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/common/protobuf_utils.hpp, line 59 > > <https://reviews.apache.org/r/6842/diff/1/?file=147306#file147306line59> > > > > why not do: > > > > if (executorId.value() != "") unfortunately, != operators are not defined for our protobufs. we should probably fix that in another review. > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/slave/state.hpp, line 41 > > <https://reviews.apache.org/r/6842/diff/1/?file=147309#file147309line41> > > > > 1. perhaps we can differentiate between files and dirs here? > > > > like: > > SLAVEID_FILEPATH > > SLAVE_PATH > > > > 2. This makes it a bit tricky to see the required formatting, what if > > we just made helpers instead that takes the necessary arguments that > > internally knows how to use these with strings::format 1) I think PATH is generic for both dirs and files. I'm not convinced having different names is better. 2) done > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 62 > > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line62> > > > > does this also apply for the empty list? done > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 77 > > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line77> > > > > would we normally just use a pointer for this? i would like to avoid pointers as much as possible :) > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 89 > > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line89> > > > > empty list here as well? done > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 180 > > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line180> > > > > I'll be fixing some of these os command to be try<nothing> since we > > never return false, might be best to still check for false here for now i don't see the advantage? > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 220 > > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line220> > > > > maybe check for false as well, even though it'll never happen ditto > On Sept. 1, 2012, 12:19 a.m., Ben Mahler wrote: > > src/slave/state.cpp, line 222 > > <https://reviews.apache.org/r/6842/diff/1/?file=147310#file147310line222> > > > > formatting looks ok to me. bad, RB formatting? - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6842/#review10959 ----------------------------------------------------------- On Aug. 29, 2012, 11:09 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/6842/ > ----------------------------------------------------------- > > (Updated Aug. 29, 2012, 11:09 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 aaceee3 > src/common/protobuf_utils.hpp PRE-CREATION > src/slave/slave.hpp 10c537b > src/slave/slave.cpp 4efd41e > 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 b1eceb3 > > Diff: https://reviews.apache.org/r/6842/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
