> On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote: > > src/slave/state.cpp, line 34 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line34> > > > > 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?
That was the original intention, but I agree with BenM that having helpers would be useful for users of this interface. Because, that would obviate the need for them to remember/understand the path semantics. For eg: they dont have to know that the 1st argument should be string template, 2nd should SlaveID etc. As I mentioned in BenM's review, I would prefer to also have helpers for glob. But I'm punting on that for C++11. Makes sense? > On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote: > > src/slave/state.cpp, lines 168-171 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line168> > > > > 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. done > On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote: > > src/slave/state.cpp, lines 210-211 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line210> > > > > 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); done, at the cost of 1 character over the limit abuse. > On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote: > > src/slave/state.cpp, line 232 > > <https://reviews.apache.org/r/6842/diff/2/?file=153053#file153053line232> > > > > > > state.frameworks[frameworkId].executors[executorId].runs[run].tasks.insert(taskId); done > On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote: > > src/tests/slave_state_tests.cpp, line 51 > > <https://reviews.apache.org/r/6842/diff/2/?file=153054#file153054line51> > > > > When is this possible? Seems like you'd want to do a CHECK here. it was needed because of the way i initialized it, fixed. > On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote: > > src/tests/slave_state_tests.cpp, lines 41-45 > > <https://reviews.apache.org/r/6842/diff/2/?file=153054#file153054line41> > > > > Can these guys be const? Set in constructor? I'm not sure how to initialize protobuf objects inside an initializer list? > On Sept. 13, 2012, 7:09 p.m., Benjamin Hindman wrote: > > src/tests/slave_state_tests.cpp, line 58 > > <https://reviews.apache.org/r/6842/diff/2/?file=153054#file153054line58> > > > > I smell 'Option<string> rootDir;' instead of checking for empty string > > (more explicit). ;) n/a, with the latest refactor. - Vinod ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/6842/#review11489 ----------------------------------------------------------- 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 > >
