----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/19007/#review37303 -----------------------------------------------------------
Looks like this at least needs to depend on Mutex? I'd like to better understand the full picture around the log API with respect to how long operations can take, whether operations can take forever, whether we need timeouts, and at what layer these timeouts should live. Looks great, but there are two bugs now that we've changed the Option::get semantics! src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69493> How about adding a TODO to mention that we're returning 'false' for a case where we're not sure whether the version has changed. Maybe comment on what we could do with respect to re-enqueing the operation behind another start(), using backoff to prevent fighting, etc.. src/state/log.cpp <https://reviews.apache.org/r/19007/#comment68760> Maybe a newline between each group of related continuations? src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69494> "the ability"? src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69504> Why is this a TODO? src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69506> Can these be 'const' or did you want assignability? src/state/log.cpp <https://reviews.apache.org/r/19007/#comment68755> It doesn't look like index can be some here, should we CHECK that it is none to simplify this? src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69575> These two comments look a bit redundant? src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69530> 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? src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69578> Or we s/truncated/beginning/ to make the meaning a bit more clear? It's a little unintuitive that we set truncated to the beginning (there may not be a truncation yet). Seems like keeping an up-to-date 'beginning' position might be a bit easier to understand. src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69579> s/Futhermore/Furthermore/ src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69580> We might as well be consistent and put 'then' on a newline, ditto in the other start() cases :) src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69587> This line is broken with the new const & Option::get semantics! src/state/log.cpp <https://reviews.apache.org/r/19007/#comment69591> This line is broken with the new Option::get const & semantics! src/state/log.cpp <https://reviews.apache.org/r/19007/#comment68752> Again we're going from a 'set' to 'vector' for names(), seems like the API should return a unique collection of names, alas :) src/tests/state_tests.cpp <https://reviews.apache.org/r/19007/#comment68751> Can you remove the std:: qualifiers for std::string and std::set here? - Ben Mahler On March 11, 2014, 2:53 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/19007/ > ----------------------------------------------------------- > > (Updated March 11, 2014, 2:53 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 384b3122b61294401ba4a894c06e985d9fc2fb1e > src/messages/state.proto 7f7a8a505d6f24b01fec0c3ad47b0e15b2b17ffa > src/state/log.hpp PRE-CREATION > src/state/log.cpp PRE-CREATION > src/tests/log_tests.cpp b368cdec6a81a8ddeb6fc14d4723a09797a6b0a1 > src/tests/state_tests.cpp bc6c91439a50e7b93077f5b1e66b0419860cb35b > > Diff: https://reviews.apache.org/r/19007/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
