----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7655/#review12673 -----------------------------------------------------------
Micro review, haven't looked at the tests yet. include/mesos/mesos.proto <https://reviews.apache.org/r/7655/#comment27014> s/better recover/recover? Or is there some form of recovery right now? include/mesos/mesos.proto <https://reviews.apache.org/r/7655/#comment27018> Set a default value of false? Otherwise, I'm a bit uncomfortable using info.checkpoint() without checking info.has_checkpoint() first. src/messages/messages.proto <https://reviews.apache.org/r/7655/#comment27015> Remove TODO since we can't make any added fields required. Otherwise we could have issues during live rolls, no? The unfortunate idiom I remember from google was: optional bool checkpoint = 8; // Required. We should probably do this here and elsewhere when we add new "required" fields. src/slave/slave.hpp <https://reviews.apache.org/r/7655/#comment27016> s/params/parameters src/slave/slave.hpp <https://reviews.apache.org/r/7655/#comment27020> Can you document what the medaDirectory is? src/slave/slave.hpp <https://reviews.apache.org/r/7655/#comment27017> Just a reminder that this will go away when you rebase off trunk, once my uuid run change goes in. src/slave/slave.hpp <https://reviews.apache.org/r/7655/#comment27019> Was there a reason this was left as a TODO? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment27021> It seems like this log line will come out difficult to read, unless we add a newline, or separate log line. src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment27022> 2 spaces instead of 4, if not line continuing from an open paren. src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment27023> ditto src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment27024> s/,// src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment27026> Why do you loop through all frameworks when this function takes the frameworkId? Is there any way this function can take the executorId as well to avoid looping at all? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment27030> Any reason for dirname? Could simply be: if (!os::exists(path.get())) { Try<Nothing> result = os::mkdir(path.get()); .. } - Ben Mahler On Oct. 20, 2012, 3:04 a.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7655/ > ----------------------------------------------------------- > > (Updated Oct. 20, 2012, 3:04 a.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Integrated SUM into slave. > > > Diffs > ----- > > include/mesos/mesos.proto 72fccdec033b287aa58d1e23b07f9ef6082773bc > src/Makefile.am cf9364ea0418ec30a75b1774f32bda8de6e031ac > src/messages/messages.proto 4e0538fe929f9091e5cdd4a1bb017d836df52a3e > src/slave/gc.cpp 679504e51922c5ea54a476d061262e8e8f2aa4b6 > src/slave/paths.hpp 48a7be0ae33785206ebb1985647178224d6bcfb8 > src/slave/slave.hpp 343c353027dbd2a7c1be8cee99a1d59367169177 > src/slave/slave.cpp 5af7464aae17c00a0e707421982d7cb055aabc6c > src/tests/status_updates_manager_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/7655/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
