> On March 20, 2014, 1:03 a.m., Ben Mahler wrote: > > src/state/log.cpp, line 340 > > <https://reviews.apache.org/r/19007/diff/1/?file=515951#file515951line340> > > > > This line is broken with the new const & Option::get semantics!
That is nasty nasty nasty, looking forward to rvalue references because this would have been a sufficient reason not to make all those optimizations yet! Thanks for the careful review Ben! > On March 20, 2014, 1:03 a.m., Ben Mahler wrote: > > src/state/log.cpp, lines 180-183 > > <https://reviews.apache.org/r/19007/diff/1/?file=515951#file515951line180> > > > > It doesn't look like index can be some here, should we CHECK that it is > > none to simplify this? Hmm, I don't think you fully groked this part of the code: since 'start' can be called multiple times after the first time 'index' will be some. I updated the comment. > On March 20, 2014, 1:03 a.m., Ben Mahler wrote: > > src/state/log.cpp, lines 111-115 > > <https://reviews.apache.org/r/19007/diff/1/?file=515951#file515951line111> > > > > Why is this a TODO? Changed to just a comment. > On March 20, 2014, 1:03 a.m., Ben Mahler wrote: > > src/state/log.cpp, lines 124-125 > > <https://reviews.apache.org/r/19007/diff/1/?file=515951#file515951line124> > > > > Can these be 'const' or did you want assignability? Changed to 'const', thanks. > On March 20, 2014, 1:03 a.m., Ben Mahler wrote: > > src/state/log.cpp, line 229 > > <https://reviews.apache.org/r/19007/diff/1/?file=515951#file515951line229> > > > > Why not add an 'else' clause here to catch unknown Operation types? I > > can see this code making sense if we planned on deprecating old 'Operation' > > types? Changed to a 'switch'. > On March 20, 2014, 1:03 a.m., Ben Mahler wrote: > > src/state/log.cpp, lines 452-459 > > <https://reviews.apache.org/r/19007/diff/1/?file=515951#file515951line452> > > > > Again we're going from a 'set' to 'vector' for names(), seems like the > > API should return a unique collection of names, alas :) Okay, see https://reviews.apache.org/r/19835. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19007/#review37303 ----------------------------------------------------------- On March 31, 2014, 7:11 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19007/ > ----------------------------------------------------------- > > (Updated March 31, 2014, 7:11 a.m.) > > > Review request for mesos, Ben Mahler and Jie Yu. > > > Repository: mesos-git > > > Description > ------- > > This is a combination of both https://reviews.apache.org/r/17423 and > https://reviews.apache.org/r/17424 which have been merged to make it easier > to make changes. > > > Diffs > ----- > > src/Makefile.am 47d03b393f68ebd4e9eba3206798a939078023c0 > src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa > src/state/log.hpp PRE-CREATION > src/state/log.cpp PRE-CREATION > src/tests/state_tests.cpp d0e084070c566ee7d751a8e1279772e05b966145 > > Diff: https://reviews.apache.org/r/19007/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
