----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7655/#review14450 -----------------------------------------------------------
src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30744> Maybe point out that this allows this to be called directly, but in particular we'd like it to allow Slave::finalize() to call it. Something like: // Allow direct calls to shutdownFramework (in particular, from Slave::finalize()). // If called from a message, ensure it's from the leading master. src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30745> Can you log who asked for the shutdown? i.e. add 'from' to the log when it's set, seems useful for debugging. src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30747> I don't see where the framework gets removed from the framework map here, can you add a comment as to where that happens? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30748> Is this Future<bool> necessary? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30749> Can you add the future failure reason to this log message? Do you want to also check for discarded? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30757> Is logging an error ok here? Is it ok to proceed, or do you want to bail out and take some action instead of calling update? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30759> /s/" : "/": " for consistency with our other logs src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30760> ditto on future failure reason and discarded src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30762> What cleanup? The gc? The destroyExecutor? Both? src/slave/slave.cpp <https://reviews.apache.org/r/7655/#comment30763> ditto on failure reason and discarded check src/tests/status_update_manager_tests.cpp <https://reviews.apache.org/r/7655/#comment30764> Can you add a comment on what each test is exercising? It's a bit hard for me to follow this and our other tests, since they are quite verbose. - Ben Mahler On Dec. 12, 2012, 11:13 p.m., Vinod Kone wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7655/ > ----------------------------------------------------------- > > (Updated Dec. 12, 2012, 11:13 p.m.) > > > Review request for mesos, Benjamin Hindman and Ben Mahler. > > > Description > ------- > > Integrated SUM into slave. > > > Diffs > ----- > > include/mesos/mesos.proto 38235157d45bdccb676e5c3241c21b585a6f8801 > src/Makefile.am c94736df660a25b58dc47c07d9c56c3c26152a66 > src/slave/gc.cpp fe22f9b557215514e3d432d36af9dc0c377c437b > src/slave/paths.hpp fbf3fd84fb8f2590311b18d2afec2d2e0d30ef0a > src/slave/slave.hpp e9f7b659ca2860501840b3d01e69915ebd162039 > src/slave/slave.cpp 9755b46f97173d6fcc9ab1fd63e0e4814b3bc018 > src/slave/status_update_manager.hpp PRE-CREATION > src/tests/master_tests.cpp 948ab5dff34eeba1f3ce593a864ddf282c8b19ed > src/tests/status_update_manager_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/7655/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Vinod Kone > >
