----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6842/#review10959 -----------------------------------------------------------
partial review before heading out for the day src/common/protobuf_utils.hpp <https://reviews.apache.org/r/6842/#comment23529> just directly return? return (state == TASK_FINISHED || state == TASK_FAILED || state == TASK_KILLED || state == TASK_LOST); src/common/protobuf_utils.hpp <https://reviews.apache.org/r/6842/#comment23530> why not do: if (executorId.value() != "") src/slave/state.hpp <https://reviews.apache.org/r/6842/#comment23531> 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 src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment23532> again, it would be great to wrap these format's up into helpers, seems prone to making a mistake this way src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment23533> does this also apply for the empty list? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment23535> would we normally just use a pointer for this? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment23536> empty list here as well? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment23537> 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 src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment23538> maybe check for false as well, even though it'll never happen src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment23539> formatting - Ben Mahler 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 > >
