----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6842/#review11489 -----------------------------------------------------------
src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24621> In the end, I think I prefer just calling strings::format instead of get*Path. Reasoning: (1) You have to do it that way when you want to glob anyway, so it's kind of nice to not have two different ways to do it. (2) The real differences in characters are: 'get*Path' => 'strings::format(*_PATH,' and also the .get() at the end, but the .get() will go away and the rest is not so different. Thoughts? src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24611> I think for all of these you could do: foreach (const string& path, frameworks.get()) { FrameworkID frameworkId; frameworkId.set_value(os::basename(path)); } So, kill the ePath, extra const string&, etc. src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24614> I know, I know, it seems crazy, but I'd prefer: state.frameworks[frameworkId].executors[executorId].latest = std::max(run, state.frameworks[frameworkId].executors[executorId].latest); src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24615> Kill this and embed (see comment below). src/slave/state.cpp <https://reviews.apache.org/r/6842/#comment24608> state.frameworks[frameworkId].executors[executorId].runs[run].tasks.insert(taskId); src/tests/slave_state_tests.cpp <https://reviews.apache.org/r/6842/#comment24604> Can these guys be const? Set in constructor? src/tests/slave_state_tests.cpp <https://reviews.apache.org/r/6842/#comment24605> When is this possible? Seems like you'd want to do a CHECK here. src/tests/slave_state_tests.cpp <https://reviews.apache.org/r/6842/#comment24606> I smell 'Option<string> rootDir;' instead of checking for empty string (more explicit). ;) third_party/libprocess/include/stout/format.hpp <https://reviews.apache.org/r/6842/#comment24601> Thank you! third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6842/#comment24602> s/file/file: / third_party/libprocess/include/stout/os.hpp <https://reviews.apache.org/r/6842/#comment24603> s/directory/directory:/ - Benjamin Hindman 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 > >
