----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/16724/#review35689 -----------------------------------------------------------
Ship it! Test has a few small cleanups needed. src/master/master.cpp <https://reviews.apache.org/r/16724/#comment66404> We don't try and wrap by putting the brace on a newline, please pull the brace back and wrap after the comma: foreach (const Archive::Framework& completedFramework_, completedFrameworks_) { ... src/master/master.cpp <https://reviews.apache.org/r/16724/#comment66405> See wrapping comment above. src/slave/slave.cpp <https://reviews.apache.org/r/16724/#comment66406> See wrapping comment above please. src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66384> Please move '{' to newline and use const & for parameters (and did you really want to return a 'const string'?). Also, please add a healthier comment that no only should we "use use a real JSON parser" but this will not return the entire value in the event that the value is an object or an array since it will stop at the first comma. src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66385> s/idx/index/ src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66390> Can you leave a comment as to why you've created this here, how it differs from 'createTask' in mesos.hpp and perhaps even a TODO to move this into mesos.hpp? My guess is that you could easily overload 'createTask' in mesos.hpp and you get this behavior or you get the 'command' behavior if you pass a 'command' string. src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66396> Wrap this please (see other tests). src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66397> Do you need 'framework'? Why not just: EXPECT_NE("", frameworkId.get().value()); src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66401> Is this supposed to be master.get() or slave.get()? Also, there is a lot of code here, can you put a newline between the masterState "stanza" and the slaveState "stanza" to make it more readable? src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66400> Is this supposed to be master.get() or slave.get()? Also, add a newline here like above. src/tests/fault_tolerance_tests.cpp <https://reviews.apache.org/r/16724/#comment66380> Go ahead and put each argument on a newline when you wrap. - Benjamin Hindman On Feb. 19, 2014, 2:07 a.m., Adam B wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/16724/ > ----------------------------------------------------------- > > (Updated Feb. 19, 2014, 2:07 a.m.) > > > Review request for mesos, Benjamin Hindman, Ben Mahler, Niklas Nielsen, and > Vinod Kone. > > > Bugs: MESOS-767 > https://issues.apache.org/jira/browse/MESOS-767 > > > Repository: mesos-git > > > Description > ------- > > Added completed frameworks/tasks to slave re-registration. > Fixes MESOS-767. > > Additional issues discovered during investigation: > - MESOS-905: Remove Framework.id in favor of FrameworkInfo.id > - MESOS-906: Last task in Completed Framework never graduates from > terminatedTasks to completedTasks. > - Completed frameworks/executors/tasks are stored in circular buffers, > and these may overflow in different orders on different slaves. > BenH proposes an archive to replace these circular buffers. > > > Diffs > ----- > > include/mesos/scheduler.hpp 2e4707e > src/master/master.hpp 7649737 > src/master/master.cpp 77872ec > src/messages/messages.proto 922a8c4 > src/slave/slave.cpp 2d21e16 > src/tests/fault_tolerance_tests.cpp 60e06cc > src/tests/mesos.hpp d7bdaee > > Diff: https://reviews.apache.org/r/16724/diff/ > > > Testing > ------- > > make check; manually failed-over a master, watched the slave reregister its > completed frameworks, web UI shows completed tasks and stdout/stderr. > Added a new unit/integration test to verify the expected behavior. > > > Thanks, > > Adam B > >
